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

Bugfix/use media recorder #1474

Merged
merged 1 commit into from
Sep 26, 2022
Merged

Bugfix/use media recorder #1474

merged 1 commit into from
Sep 26, 2022

Conversation

laineyhm
Copy link
Collaborator

@laineyhm laineyhm commented Sep 21, 2022

Description

MediaRecorder is the most current API for audio recording in browser. With MediaRecorder, Language Forge will capture audio in WEBM containers with Opus codec.

ffmpeg is added as a dependency to perform audio conversions. The audio files are converted and stored as either .wav or .mp3 files to accommodate send/receive with Fieldworks software. If the recording is short enough (<6 s), based on settings in conversion to WAV, it will be within the 1MB file size limit. Recordings longer than 6 seconds are converted to MP3.

webm-fix-duration is added as a small (25 KB) dependency. Currently, navigator.mediaDevices.getUserMedia with MediaRecorder returns WEBM file blobs that have no duration metadata. This affects the display of the sound player as well as decisions for encoding. The webm-fix-duration package takes care of that by appending the duration data to the blob so it may be accessed by the app.

Fixes #1382
Fixes #1381
Fixes #1399

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

The UI works as before. In the console here, see saved files converted to .wav and .mp3.

image

Testing on your branch

  • Test the following in Chrome, Firefox, Edge, and Safari, as well as on iOS and Android mobile devices.
    To test on mobile, install ngrok and run:
    ./ngrok http http://localhost
    in a bash terminal. The same command with https://localhost may not work, so be careful to try http://localhost in particular. You will get a URL that you can use for testing from a mobile device.
  1. Open the HTML docker container in lf-app. Open assets/lexicon/[project name]/audio. This is where all saved recordings and uploaded audio files go.
  2. Record a short (<6 second) clip online, play it back, and save it. See if it is found in the container twice: once in the .webm format and once as a .wav file.
  3. Record a longer (>6 second) clip online, play it back, and save it. See if it is found in the container twice: once in the .webm format and once as an .mp3 file.
  4. Upload a long and a short clip of each of these file types: .ogg, .flac, .m4a. See if they are found in the container twice, with conversion to .wav for the short files and .mp3 for the long files.
  5. Upload an .mp3 and a .wav file to see if these files are stored in the container "as-is"; no conversion, no second copy. Here is a folder of test files to use: Audio Test Upload Files.zip
  • See if files added online come into FLEx via Send/Receive.

Checklist

  • I have performed a self-review of my own code
  • I have reviewed the title/description of this PR which will be used as the squashed PR commit message
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works -- have not done this, maybe I can learn.

qa.languageforge.org testing

Reviewers: add/replace your name below and check the box to sign-off/attest the feature works as expected on qa.languageforge.org

@github-actions
Copy link

github-actions bot commented Sep 21, 2022

Unit Test Results

368 tests   368 ✔️  9s ⏱️
    1 suites      0 💤
    1 files        0

Results for commit 9c7a238.

♻️ This comment has been updated with latest results.

@megahirt
Copy link
Collaborator

Also fixes #1381

Great job @laineyhm !

The unit tests are failing because we need to build and push a base-php image that contains ffmpeg. We can do that together tomorrow.

Copy link
Contributor

@longrunningprocess longrunningprocess left a comment

Choose a reason for hiding this comment

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

there's a lot of complexity in src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts, please be sure we've covered testing well in here.

@longrunningprocess
Copy link
Contributor

Also fixes #1381

Great job @laineyhm !

The unit tests are failing because we need to build and push a base-php image that contains ffmpeg. We can do that together tomorrow.

I took care of that (sorry if it was meant to be a teaching moment) https://github.com/sillsdev/web-languageforge/actions/runs/3099569423/jobs/5018860502

@longrunningprocess
Copy link
Contributor

unit test failure looks like an inheritance issue that needs to be worked out:

PHP Fatal error:  Cannot redeclare Api\Model\Languageforge\Lexicon\Command\getDuration() (previously declared in /var/www/html/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php:98) in /var/www/html/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php on line 98

@megahirt
Copy link
Collaborator

unit test failure looks like an inheritance issue that needs to be worked out:

PHP Fatal error:  Cannot redeclare Api\Model\Languageforge\Lexicon\Command\getDuration() (previously declared in /var/www/html/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php:98) in /var/www/html/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php on line 98

Yeah, I saw that too. Something is not right with the PHP code as well. Lainey hasn't been running the unit tests locally so this is probably # the first time she's seeing it.

@megahirt
Copy link
Collaborator

I took care of that (sorry if it was meant to be a teaching moment) https://github.com/sillsdev/web-languageforge/actions/runs/3099569423/jobs/5018860502

Totally fine! Thanks for taking care of that.

@laineyhm laineyhm enabled auto-merge (squash) September 22, 2022 06:26
Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

Great job!!! I am very pleased to see that you were able to overcome the challenges in the angular component that weren't working, and ended up getting it all working.

I'll approve this PR when you:

  • sanitize the shell arg
  • keep around the webm file as well, even though we don't point to it directly

Future TODO items not in this PR

  • write playwright tests that exercise the audio component. We don't have previous tests anyway, so you're breaking new ground here
  • handle conversion of uploaded files to wav/mp3 for FLEx compatibility

Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

a few more very minor changes - looking good!

@megahirt
Copy link
Collaborator

@laineyhm we are going to need some "how to test this PR" instructions in the PR description so others of us can test this functionality appropriately. A few scenarios include uploading a variety of formats (webm, m4a, flac, ogg, etc) and see that they get converted. Of course also in-browser recording, especially on mobile.

Copy link
Collaborator

@megahirt megahirt left a comment

Choose a reason for hiding this comment

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

Woohoo approved! This PR has it all: Typescript, PHP, MediaRecorder, FFMPEG :)

@laineyhm
Copy link
Collaborator Author

Description updated with testing instructions

Using Media-Recorder for recording. play/pause bug

Fixed error with play/plause. Still debugging playback

Another work-in-progress commit. Rearranged some

Committing to save progress; debugging sound-player

Playback is working, but NaN still shows up at the beginning sometimes

Put the scope applications back in

Audio source updates immediately. Playback and slider working.

Tweaks to slider; Practicing uploads

More tweaks to the slider

Fix bug (remove out-of-place console log)

With the added ffmpeg dependency in the php server, converts .ogg recordings to .wav (<6.25 seconds) or .mp3 (>6.25 seconds) for use with Flex

cleaned up code

Conversions for FLEx implemented; mp3/wav based on audio duration. Cleaned up comments.

Fixed php function error. Removing unused experimental listeners, html attributes, else statements, and dependencies

Small cleanup

Adding @types/dom-mediacapture-record for MediaRecorder types, among other suggested edits

Saving the recorded/uploaded file as well as the converted one. Allowing for FLAC and OGG uploads. Sanitizing shell args.

Took out code that was removing duplicate files with different formats

Sanitizing shell args in all places; making code Prettier

Apply suggestions from code review

Co-authored-by: Christopher Hirt <[email protected]>
@laineyhm laineyhm merged commit f3eb6ea into develop Sep 26, 2022
@laineyhm laineyhm deleted the bugfix/use-media-recorder branch September 26, 2022 14:51
@LHayashi
Copy link

Thanks for getting this working so that recording can take place in Language Forge! That said, it does pain me to store recordings as compressed mp3s which doesn't meet the standard expectation for language documentation. It'd be nice to see that 1MB limit increased : )

@longrunningprocess
Copy link
Contributor

longrunningprocess commented Sep 28, 2022

staging tests

Used chris-test-sr1

Chrome (105) on macOS (12.6)

🧪 < 6s with playback and save ✅

Playback was great and files were there:

image

🧪 > 6s with playback and save ✅

Playback was great and files were there:

image

🧪 delete something that was added/converted and ensure all files are removed ❌

I removed a "long-clip", the .mp3 was removed but the .m4a remained. If we don't remove all files (by the same name I suppose) we'll end up with a "leak" on the filesystem, e.g., ...long-clip in this case:

image

🧪 upload long and short .ogg, .flac, .m4a ✅

Upload and playback were great on all and files were there:

image

🧪 upload long and short .wav, .mp3 ✅

Upload and playback were great on both and files were not converted:

image

@megahirt
Copy link
Collaborator

Thanks for getting this working so that recording can take place in Language Forge! That said, it does pain me to store recordings as compressed mp3s which doesn't meet the standard expectation for language documentation. It'd be nice to see that 1MB limit increased : )

I agreed 100% @LHayashi . As it stands, FLEx only supports wav and mp3. Up until now, LF only recorded in mp3 but you could upload wav or m4a. With this change, we support a bunch more formats AND for the first time encode as wav or mp3 (for FLEx compatibility) based upon recording length.

The main problem as you know is the 1MB limit, which is a chorus limit, not a LF limit. But this is something we need to push for a resolution.

@megahirt
Copy link
Collaborator

c.f. sillsdev/chorus#277

@megahirt
Copy link
Collaborator

@laineyhm see the comments by @rmunn in sillsdev/chorus#277 that talk about the exact length of the wav file to fit within the size limit. Robin comes up with 5.6 seconds whereas you came up with 6 seconds. Do you want to double-check whether a 6 second wav file fits within 1MB? I acknowledge the "line" for 1MB is not clear here. I would suggest that we go with Robin's assessment as it it's lower. One thing would be to check by uploading a 5.9 second wav file to see whether it can successfully be send/received.

@laineyhm
Copy link
Collaborator Author

image
When converting to .wav I restricted the output bitrate to a max 165K bits/s. 165,000 bits/s * 6 s gives 990,000 bits, which I thought was just under a MB. But, alas, a byte is 8 bits, so a MB is actually a lot bigger. The other piece is that my max output bitrate was too low; I must have mixed something up. It should be closer to 1400KB/s.
8 million bits divided by 1400 KB/s gives ~5.6 s. Sorry for the confusion! Should I open a PR with this fix ?

@laineyhm
Copy link
Collaborator Author

laineyhm commented Sep 29, 2022

So far in S/R with FLEx, the audio files are coming through into FLEx, but the playback is not audible in FLEx (it's audible from the computer's media player, separately). Troubleshooting that.

@megahirt
Copy link
Collaborator

@laineyhm and I paired and discovered what we think to be the issue regarding wav files not coming through on FLEx. Also an adjustment to the bitrate per the above comment will be in a PR now in progress.

longrunningprocess pushed a commit that referenced this pull request Dec 3, 2022
lamejs was removed in #1474
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants