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

[NEW] Audio recording as mp3 and better ui for recording #9726

Merged
merged 13 commits into from
Mar 15, 2018

Conversation

kb0304
Copy link
Contributor

@kb0304 kb0304 commented Feb 15, 2018

@RocketChat/core
Closes #7844 #4552

Real-time mp3 encoding or audio message using lame.js (JS library ported from LAME Mp3 encoder)

Updated UI :

agif

@kb0304 kb0304 changed the title [NEW] mp3 audio upload [NEW] Audio upload: mp3 encoding, update UI Feb 16, 2018
@geekgonecrazy
Copy link
Contributor

@RocketChat/core can we give this some attention?

❤️ This UI change. Not sure on performance implications of libmp3lame.js I assume compatible with all recent browsers?

@geekgonecrazy
Copy link
Contributor

Instead of including the full libmp3lame.js can we use an MP3 reference? Ideally a library that we aren't developing would be brought in as a dependency via npm

@amonemian
Copy link

@kb0304 thank you for your great effort. all things are good except the deley between pressing the green "ok" button and the sending progress bar load.

What's exactly the cause of the delay? and how can I overcome that?

@kb0304
Copy link
Contributor Author

kb0304 commented Feb 19, 2018

@geekgonecrazy
Yes, it is compatible. I have tested it for Chrome and Firefox.
Can you please guide me on how to bring it in as a npm dependency and inject only the relevant methods into the worker?

@amonemian
The delay is the time taken for encoding to mp3, which starts at the end of the recording process. I will work on encoding it while recording itself, which would take care of this issue.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 19, 2018 15:03 Inactive
@rodrigok
Copy link
Member

@kb0304 the box for audio control is taking unnecessary space when not in use
image

@rodrigok
Copy link
Member

@kb0304 Can you replace the mic icon by the loading spinner while processing?

@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9726 February 19, 2018 16:24 Inactive
@rodrigok
Copy link
Member

@kb0304 It's not working on safari

@rodrigok
Copy link
Member

@kb0304 Safari 11.0.3

@geekgonecrazy
Copy link
Contributor

geekgonecrazy commented Feb 19, 2018

Can you please guide me on how to bring it in as a npm dependency and inject only the relevant methods into the worker?

@kb0304
https://www.npmjs.com/package/lame looks like contains / wraps it.

Plenty of packages in the project bring in npm packages. So lots of examples. Just define the npm package in the package.js and do an import at the top of the file you use it in.

@kb0304
Copy link
Contributor Author

kb0304 commented Feb 19, 2018

@geekgonecrazy

I am unable to use the imported package in Worker's scope. Is there any other alternative to serving the static js file from the server?
The package you mentioned provides a minified static file. How can we serve it as a static file?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 19, 2018 22:25 Inactive
@geekgonecrazy
Copy link
Contributor

geekgonecrazy commented Feb 19, 2018

You should just be able to swap this import for an import to the package and serve this file from public still. https://github.com/RocketChat/Rocket.Chat/pull/9726/files#diff-d796bdf4e59d6b846640607e356f820aR1 I see no problem continuing to serve this wrapper. Its just including the library its self and serving that up that feels dirty. :)

@kb0304
Copy link
Contributor Author

kb0304 commented Feb 19, 2018

@geekgonecrazy I agree with you. Will improve it.

@kb0304
Copy link
Contributor Author

kb0304 commented Feb 19, 2018

@rodrigok I have fixed the UI and have added a loader. I tested the Heroku deployment via Safari 11.0.3 and it worked without any issue. Could you please share steps to reproduce / details about the error?
It does have a pre-existing issue, it doesn't work in Safari when served via HTTP even on localhost.

Updated UI:
agif

@theorenck theorenck added this to the 0.63.0 milestone Feb 20, 2018
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 20, 2018 14:00 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 20, 2018 14:12 Inactive
package.json Outdated
"ip-range-check": "^0.0.2",
"jquery": "^3.3.1",
"jschardet": "^1.6.0",
"lamejs": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

Are you still using the import?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the idea was to use this instead of including the file. But per DM struggling to get the import to work in the worker.

So need to figure out how to serve the file out of the npm package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that was the purpose but I am not able to serve it from node_modules
I have removed the dependency for now.

package.json Outdated
@@ -136,9 +136,13 @@
"iconv-lite": "^0.4.19",
"image-size": "^0.6.2",
"imap": "^0.8.19",
"ip-range-check": "0.0.2",
"jquery": "^3.2.1",
"jschardet": "1.6.0",
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these duplicated lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 21, 2018 12:07 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9726 February 21, 2018 14:04 Inactive
@rodrigok rodrigok changed the title [NEW] Audio upload: mp3 encoding, update UI [NEW] Audio recording as mp3 and better ui for recording Mar 15, 2018
@rodrigok rodrigok merged commit 082cc95 into RocketChat:develop Mar 15, 2018
@kb0304 kb0304 deleted the bugfix-audio-format branch March 17, 2018 10:49
@rodrigok rodrigok mentioned this pull request Apr 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants