-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Conversation
There was a problem hiding this 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.
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
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 |
unit test failure looks like an inheritance issue that needs to be worked out:
|
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. |
Totally fine! Thanks for taking care of that. |
There was a problem hiding this 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
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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!
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/Api/Model/Languageforge/Lexicon/Command/LexUploadCommands.php
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
src/angular-app/bellows/shared/audio-recorder/audio-recorder.component.ts
Outdated
Show resolved
Hide resolved
src/angular-app/languageforge/lexicon/editor/field/dc-audio.component.ts
Outdated
Show resolved
Hide resolved
src/angular-app/languageforge/lexicon/editor/field/dc-audio.component.ts
Outdated
Show resolved
Hide resolved
@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. |
There was a problem hiding this 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 :)
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 <chris@hirtfamily.net>
5ae6114
to
9c7a238
Compare
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 : ) |
staging testsUsed Chrome (105) on macOS (12.6)🧪 < 6s with playback and save ✅Playback was great and files were there: 🧪 > 6s with playback and save ✅Playback was great and files were there: 🧪 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., 🧪 upload long and short .ogg, .flac, .m4a ✅Upload and playback were great on all and files were there: 🧪 upload long and short .wav, .mp3 ✅Upload and playback were great on both and files were not converted: |
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. |
c.f. sillsdev/chorus#277 |
@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. |
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. |
@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. |
lamejs was removed in #1474
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
Screenshots
The UI works as before. In the console here, see saved files converted to .wav and .mp3.
Testing on your branch
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.
Checklist
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