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

fix_: crash on viewing dynamic file type collectibles #21245

Merged

Conversation

smohamedjavid
Copy link
Member

fixes #20466

Summary

This PR removes the support for GIF file types for the collectibles.

Review notes

The crash is due to bloated memory consumption due to dynamic file types (such as GIF) in the list that result in low FPS.

It’s consumed so much memory. The average RAM consumption was 1.2 GB (60 FPS), if the collectables were static images. If it’s a GIF, it goes up to 2.5 GB (8 FPS).

Until we find a solid solution to handle the dynamic data types, we drop the support for GIFs and support only static images/file types.

Platforms

  • Android
  • iOS

Steps to test

Prerequisite: A profile with a lot of collectibles (GIFs)

  • Open Status
  • Log into your account
  • Navigate to the Wallet tab and then collectibles
  • Verify unsupported file is displayed for GIF-type collectibles
  • Verify that the scrolling through the list doesn't crash the app

status: ready

@smohamedjavid smohamedjavid requested a review from vkjr September 10, 2024 14:48
@smohamedjavid smohamedjavid self-assigned this Sep 10, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Sep 10, 2024

Jenkins Builds

Click to see older builds (8)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 3f347e5 #1 2024-09-10 14:53:57 ~5 min tests 📄log
✔️ 3f347e5 #1 2024-09-10 14:55:46 ~6 min android-e2e 🤖apk 📲
✔️ 3f347e5 #1 2024-09-10 14:57:27 ~8 min android 🤖apk 📲
✔️ 3f347e5 #1 2024-09-10 15:01:36 ~12 min ios 📱ipa 📲
✔️ f884430 #2 2024-09-12 13:40:54 ~4 min tests 📄log
✔️ f884430 #2 2024-09-12 13:44:04 ~7 min android-e2e 🤖apk 📲
✔️ f884430 #2 2024-09-12 13:44:53 ~8 min android 🤖apk 📲
✔️ f884430 #2 2024-09-12 13:54:17 ~17 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 92bfa2b #3 2024-09-18 12:06:17 ~5 min tests 📄log
✔️ 92bfa2b #3 2024-09-18 12:09:31 ~8 min android-e2e 🤖apk 📲
✔️ 92bfa2b #3 2024-09-18 12:09:56 ~9 min android 🤖apk 📲
✔️ 92bfa2b #3 2024-09-18 12:12:46 ~11 min ios 📱ipa 📲
✔️ c8e7227 #4 2024-09-19 10:07:54 ~5 min tests 📄log
✔️ c8e7227 #4 2024-09-19 10:11:25 ~9 min android-e2e 🤖apk 📲
✔️ c8e7227 #4 2024-09-19 10:12:43 ~10 min android 🤖apk 📲
✔️ c8e7227 #4 2024-09-19 10:14:31 ~12 min ios 📱ipa 📲

@smohamedjavid smohamedjavid force-pushed the fix/crash-on-displaying-gif-collectibles-in-list branch from 3f347e5 to f884430 Compare September 12, 2024 13:36
@smohamedjavid smohamedjavid marked this pull request as ready for review September 12, 2024 13:36
@VolodLytvynenko VolodLytvynenko self-assigned this Sep 16, 2024
@status-im-auto
Copy link
Member

86% of end-end tests have passed

Total executed tests: 7
Failed tests: 1
Expected to fail tests: 0
Passed tests: 6
IDs of failed tests: 702745 

Failed tests (1)

Click to expand
  • Rerun failed tests

  • Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Test setup failed: critical/chats/test_1_1_public_chats.py:20: in prepare_devices
        self.drivers, self.loop = create_shared_drivers(2)
    base_test_case.py:329: in create_shared_drivers
        raise e
    base_test_case.py:319: in create_shared_drivers
        test_suite_data.current_test.testruns[-1].jobs[drivers[i].session_id] = i + 1
     '_asyncio.Future' object has no attribute 'session_id'
    



    Device sessions

    Passed tests (6)

    Click to expand

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    2. test_wallet_send_eth, id: 727229

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    Device sessions

    @VolodLytvynenko
    Copy link
    Contributor

    VolodLytvynenko commented Sep 16, 2024

    Hi @smohamedjavid, unfortunately, I can't test this PR due to an existing blocker. I don't have enough collectibles to test smooth scrolling, and I don't own any GIF collectibles

    @VolodLytvynenko
    Copy link
    Contributor

    VolodLytvynenko commented Sep 18, 2024

    I've created a few dynamic collectibles (1 MP4 and 2 GIF formats), but they are currently shown as unsupported in our app. The owner of these collectibles is 0x4917d648de0c0d193a983ca6a50413e6f03d238c. Here are the links:

    MP4: Link
    GIF: Link
    GIF: Link
    At the moment, this is a blocker for testing the current PR. Do we have any criteria for dynamic files to be shown as supported in our app?
    @ulisesmac, if you have any insights, I'd really appreciate your help. What do you think?

    @smohamedjavid
    Copy link
    Member Author

    @VolodLytvynenko - Thank you for testing the PR 🙏

    GIF was the only dynamic file type which was supported. This is now removed in this PR due to performance reasons. On that note, you are expected to see the unsupported message.

    We will explore the other file types along with this GIF type and add support in later releases. 👍

    @VolodLytvynenko VolodLytvynenko force-pushed the fix/crash-on-displaying-gif-collectibles-in-list branch from f884430 to 92bfa2b Compare September 18, 2024 12:00
    @VolodLytvynenko
    Copy link
    Contributor

    @VolodLytvynenko - Thank you for testing the PR 🙏

    GIF was the only dynamic file type which was supported. This is now removed in this PR due to performance reasons. On that note, you are expected to see the unsupported message.

    We will explore the other file types along with this GIF type and add support in later releases. 👍

    @smohamedjavid Apologies. I misunderstood the PR initially. PR looks good, no issues from my side. Ready to be merged

    @ulisesmac
    Copy link
    Contributor

    @VolodLytvynenko
    We haven't found a performant way to display all media types of collectibles, AFAIK, we haven't done any deep research on the topic since we have different priorities.

    Btw, I've just checked the blocker you mentioned and I took it.

    @smohamedjavid Could you please provide more details about the performance metrics you shared?
    such as device, OS, tools to measure, steps taken, etc. We may take them as reference for other parts of the app. CC: @ilmotta

    @ilmotta
    Copy link
    Contributor

    ilmotta commented Sep 18, 2024

    Could you please provide more details about the performance metrics you shared?

    I second that @ulisesmac, good question.

    On later releases, we can add a preview like getting the first frame of a GIF (or any dynamic file type) and displaying it in the list and then on the details page, we can display it with no limitation.

    -- #20466 (comment)

    @smohamedjavid provided the solution I was thinking about while checking this PR, static thumbnails in the listing screen. Even famous webapps avoid showing multiple live media (e.g. YouTube), so I think this won't be seen as bad thing from users, probably even the opposite. With external gifs and videos we have no control and the experience can easily degrade in a mobile device.

    @smohamedjavid
    Copy link
    Member Author

    @smohamedjavid Could you please provide more details about the performance metrics you shared?
    such as device, OS, tools to measure, steps taken, etc. We may take them as reference for other parts of the app

    @ulisesmac - This performance measurement is done on the debug version of the app on an iOS simulator (iPhone 11 Pro) using the React Native performance monitor tool (Shake the device > debug menu > Show Perf Monitor). The tool will give so much insight into RAM usage and FPS (JS & UI). 60 FPS is smooth. Under 30 FPS will be choppy, and under 24 FPS will be a nightmare.

    The release build naturally tends to perform slightly better.

    @smohamedjavid provided the solution I was thinking about while checking this PR, static thumbnails in the listing screen. Even famous webapps avoid showing multiple live media (e.g. YouTube), so I think this won't be seen as bad thing from users, probably even the opposite. With external gifs and videos we have no control and the experience can easily degrade in a mobile device.

    Yes @ilmotta, I don't want to remove the GIF support completely.

    I explored a bit about pausing the GIF at least, but unfortunately, the React Native Image component or fast-image lib doesn't support this. We need to do some R&D to display a static thumbnail or something. We will definitely come back to this later, as this would improve the UX without affecting the device's performance.

    @smohamedjavid smohamedjavid force-pushed the fix/crash-on-displaying-gif-collectibles-in-list branch from 92bfa2b to c8e7227 Compare September 19, 2024 10:01
    @smohamedjavid smohamedjavid merged commit 9912136 into develop Sep 19, 2024
    6 checks passed
    @smohamedjavid smohamedjavid deleted the fix/crash-on-displaying-gif-collectibles-in-list branch September 19, 2024 10:15
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    [IOS] App crashes or experiences low performance after opening dynamic collectibles
    7 participants