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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: bundle video in app to prevent crash when not available (2nd attempt) #5941

Merged
merged 12 commits into from
Mar 16, 2023

Conversation

NicolasMassart
Copy link
Contributor

@NicolasMassart NicolasMassart commented Mar 9, 2023

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Important

This PR is the second attempt to push this change as the previous one #5886 failed because of an unexpected side effect and required revert, see #5940
This new one takes the side effect into account, see description bellow.

Description

Make the SRP video bundled in the app instead of retrieved from Github repos.
Have the subtitles embedded in the video, removing the need to load them from an external source.

1. What is the reason for the change?
App crashed when video player tried to load the remote video and remote was not available (or network down or anything)

2. What is the improvement/solution?

  • Bundling the video in the app to prevent crash when remote not available while still keeping it in the source repo with same name as the previous one for compatibility with currently in production apps
  • Added readme in video folder to explain and warn others.
  • Keep current subtitles files in the same location for compatibility with currently in production apps
  • Embed subtitles in the video directly and select track using the language 2 letters code,
  • Reduced video size from 5.7Mb to 2Mb with ffmpeg 2 passes encoding (including subtitles tracks).
  • Created an ffmpeg encoding and subtitles embedding script in scripts folder if we need to regenerate the video (in case of subtitle change, video update, ...)

The video is displayed by default on its original format on portrait mobile, meaning this video is most of the time around 5x3cm, very hard to see the quality diff when not really looking for it. Fullscreen makes it a bit more visible though.
The source video is still in the repos (but not bundled in the app) and the ffmpeg script allows to compress with higher bitrates if requested (change variables in the script).

Source video

https://github.com/MetaMask/metamask-mobile/raw/471_make_SRP_video_embeded_in_app/app/videos/recovery-phrase-source.mp4

Compressed video with embedded subs

https://github.com/MetaMask/metamask-mobile/raw/471_make_SRP_video_embeded_in_app/app/videos/recovery-phrase.mp4

App size impact

The app size is only 2 additional Mb for the video. Makes it around 27.8Mb for iOS and 274.7Mb for Android.

Issue

Relates to MetaMask/mobile-planning#471
fixes #5497

Testing

Simulator.Screen.Recording.-.iPhone.11.Pro.-.2023-03-15.at.14.12.23.mov
  • Video testing has been done manually as no e2e exists to test the video and previous attempts to handle the player in e2e failed.
  • retro compatibility tests: due to the revert, we add a step to check on currently in prod apps that the video is not broken.

Here are the tests done:

  • No network at all (plane mode) as required in issue scenario GIVEN: User has NO access to internet (airplane mode): the app has a system preventing it to be used and requires the user to turn network on, so not relevant.
  • Slow network: because video and subtitles are now bundled in the app, none is impacted.
  • Tested video from the multiple places it plays in the app: onboarding, settings.
  • Tested with the previous version of app (before change in code to have bundled video) that the video and subs still work when path points to this PR branch 馃槗 making sure the name and path of the video and subtitles are ok (using console logs) and that it will still work for legacy SRP video code.
     LOG  video_source_uri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/recovery-phrase.mp4?raw=true
     LOG  getSubtitleUri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/subtitles/secretPhrase/subtitles-en.vtt?raw=true
    ...
     LOG  video_source_uri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/recovery-phrase.mp4?raw=true
     LOG  getSubtitleUri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/subtitles/secretPhrase/subtitles-de.vtt?raw=true
    ...
     LOG  video_source_uri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/recovery-phrase.mp4?raw=true
     LOG  getSubtitleUri https://github.com/MetaMask/metamask-mobile/blob/471_make_SRP_video_embeded_in_app/app/videos/subtitles/secretPhrase/subtitles-hi.vtt?raw=true
    

Test artifacts

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@github-actions
Copy link
Contributor

github-actions bot commented Mar 9, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

* update urls constants
* restore original video name
* add a readme in video folder to clarify and provide video encoding options
app/constants/urls.ts Outdated Show resolved Hide resolved
- added an ffmpeg script to use when we want to regenerate the video
- updated readme instructions
removed useless SRP video constants and code related to external subs.
@NicolasMassart NicolasMassart force-pushed the 471_make_SRP_video_embeded_in_app branch from 2aa674c to 4e99678 Compare March 15, 2023 13:25
app/util/video/index.test.ts Outdated Show resolved Hide resolved
app/util/video/index.ts Outdated Show resolved Hide resolved
@NicolasMassart NicolasMassart marked this pull request as ready for review March 15, 2023 13:52
@NicolasMassart NicolasMassart requested a review from a team as a code owner March 15, 2023 13:52
@NicolasMassart NicolasMassart added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 15, 2023
app/videos/README.md Outdated Show resolved Hide resolved
app/videos/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@sethkfman sethkfman left a comment

Choose a reason for hiding this comment

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

Minor comments

app/declarations.d.ts Show resolved Hide resolved
app/components/Views/MediaPlayer/index.js Show resolved Hide resolved
scripts/compress_and_add_sub_in_video.sh Show resolved Hide resolved
scripts/compress_and_add_sub_in_video.sh Outdated Show resolved Hide resolved
app/videos/README.md Show resolved Hide resolved
scripts/compress_and_add_sub_in_video.sh Show resolved Hide resolved
@Cal-L Cal-L added the release-6.3.0 PR for release 6.3.0 label Mar 15, 2023
@Cal-L Cal-L removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 15, 2023
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

LGTM

@Cal-L Cal-L added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Mar 15, 2023
@NicolasMassart NicolasMassart merged commit 6118b13 into main Mar 16, 2023
@NicolasMassart NicolasMassart deleted the 471_make_SRP_video_embeded_in_app branch March 16, 2023 11:55
@github-actions github-actions bot locked and limited conversation to collaborators Mar 16, 2023
Copy link
Member

@Andepande Andepande left a comment

Choose a reason for hiding this comment

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

  • Tested on bitrise with IOS & Android devices
  • Video together with subtitles load fine even when network is throttled
  • Verified video loads correctly with appropriate subtitles in different Languages [en, es, hi]
  • Verified with @NicolasMassart production issue that occurred on previous merge has been rectified.

@Andepande Andepande added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Mar 16, 2023
@gauthierpetetin gauthierpetetin added the team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead label Feb 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done release-6.3.0 PR for release 6.3.0 team-mobile-ux DEPRECATED: please use "team-wallet-ux" label instead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bundle the onboarding SRP explaining video in the app build
6 participants