Push to talk #690

aviraldg · 2017-02-11T13:02:32Z

No description provided.

matrixbot · 2017-02-11T13:02:34Z

Can one of the admins verify this patch?

dbkr · 2017-02-13T11:29:12Z

@matrixbot test this please

dbkr · 2017-02-13T11:24:29Z

package.json

@@ -62,6 +62,7 @@
    "linkifyjs": "^2.1.3",
    "lodash": "^4.13.1",
    "matrix-js-sdk": "matrix-org/matrix-js-sdk#develop",
+    "msr": "^1.3.4",


I guess this basically switches between getusermedia and web audio which means we get Safari support? I'm a little hesitant to grow the bundle size by another 22KB.

Oh, looks like you still need to call getUserMedia manually, so it won't make it automatically work in Safari.

dbkr · 2017-02-13T11:54:49Z

src/components/views/rooms/AudioRecorder.js

+      });
+    } else {
+      try {
+        const mediaStream = await navigator.mediaDevices.getUserMedia({


Should probably check for this and display a "not supported" message rather than just the normal, "failed to start recording".

dbkr · 2017-02-13T11:58:54Z

src/components/views/rooms/AudioRecorder.js

+  onRecordClicked = async () => {
+    if(this.state.recording && this.mediaStream) {
+      this.recorder.stop();
+      window.ConcatenateBlobs(this.blobs, this.blobs[0].type, (blob) => {


Ew. I dislike this library that pulls in the completely non-commonjs ConcatenateBlobs which just shoves itself on the window object, for one fairly simple function.

I hate it too, but used it for a quick first version. I'll get rid of it.

dbkr · 2017-02-13T11:59:45Z

src/components/views/rooms/AudioRecorder.js

+    if(this.state.recording && this.mediaStream) {
+      this.recorder.stop();
+      window.ConcatenateBlobs(this.blobs, this.blobs[0].type, (blob) => {
+        console.log(blob);


Probably don't want to leave this in

dbkr · 2017-02-13T13:22:25Z

src/components/views/rooms/AudioRecorder.js

+  onUserMediaSuccess = (mediaStream: MediaStream) => {
+    this.mediaStream = mediaStream;
+    this.recorder = new MediaStreamRecorder(this.mediaStream);
+    this.recorder.recorderType = class extends MediaStreamRecorder.StereoAudioRecorder {


If this were a named class you could call it MonoAudioRecorder which might make it more obvious what's going on.

Won't need this once that library's gone.

dbkr · 2017-02-13T13:23:15Z

src/components/views/rooms/AudioRecorder.js

+  render() {
+    const TintableSvg = sdk.getComponent("elements.TintableSvg");
+    return (
+      <div key="controls_ptt" className="mx_MessageComposer_voicecall"


This should be an AccessibleButton nowadays

Oh. I'll check for other places where it's used -- MessageComposerInput itself does not seem to have been updated.

dbkr · 2017-02-13T13:28:24Z

src/components/views/messages/MAudioBody.js

        return (
            <span className="mx_MAudioBody">
-                <audio src={contentUrl} controls />
+                <audio src={contentUrl} controls autoPlay={shouldAutoPlay} />


I don't think we can allow auto-playing audio globally: there's too much scope for abuse. This would also presumably play all files as you scrolled past them in the backlog.

True, but I'd prefer to keep this simple and iterate on the logic rather than have this PR stuck forever (given the demand.)

What I had in mind:

Only autoplay if the current user has also sent a PTT message in the same room (so you need to send one first to have others' messages autoplay to you)

Play automatically when you receive a message (and the above condition is satisfied)

Queue up newer messages as they come in, if there's something already playing

(this can't work with the autocomplete attribute though)

Also, while I'm currently reusing the microphone icon, it might be worth considering if we want to change it.

This would also presumably play all files as you scrolled past them in the backlog

Wouldn't this also play sounds as they get rendered. So when I log in, or switch rooms? That sounds quite terrifying.

Perhaps MAY autoplay?

dbkr · 2017-02-13T13:31:36Z

Generally looking nice though, lots of people asking for this feature! Unsure why babel dislikes your flow syntax.

(fixes element-hq/element-web#1358)

Also, some usability enhancements

dbkr · 2017-02-16T17:49:21Z

src/components/views/rooms/AudioRecorder.js

+import Modal from '../../../Modal';
+import ErrorDialog from '../dialogs/ErrorDialog';
+
+export default class AudioRecorder extends React.Component {


4 space indents please, for consistency with the rest of the project

dbkr · 2017-02-16T17:51:28Z

src/components/views/rooms/AudioRecorder.js

+      if(!navigator.mediaDevices || !MediaRecorder) {
+        Modal.createDialog(ErrorDialog, {
+          description: `Sorry, but the features required for push 
+ to talk messages are not supported by your browser`,


I think ErrorDialog requires title, button and focus: https://github.com/matrix-org/matrix-react-sdk/blob/master/src/components/views/dialogs/ErrorDialog.js#L33

But it also has defaults for those properties.

dbkr · 2017-02-16T17:57:30Z

src/components/views/rooms/AudioRecorder.js

+  onUserMediaSuccess = (mediaStream: MediaStream) => {
+    this.mediaStream = mediaStream;
+
+    this.mediaRecorder = new MediaRecorder(mediaStream);


Leaving out the options parameter, in particular mimeType, seems unwise, given the default is likely to vary between browsers. What format are we expecting here? Bear in mind we'll need to spec this so it can be implemented cross-client, we'll need to nail down what audio formats clients should expect.

There is the option to check for supported mimeTypes: MediaRecorder.isTypeSupported(). Expected is "audio/webm;codecs=opus" for Chrome and "audio/ogg;codecs=opus" for Firefox. (Keep in mind that Safari doesn't support MediaRecorder.) Seems like Firefox can only record ogg audio and Chrome only WebM audio (see https://stackoverflow.com/questions/35466078/specifying-codecs-with-mediarecorder). Both browser can play both file types though, so that's not a problem.

dbkr · 2017-02-16T17:59:33Z

src/components/views/rooms/AudioRecorder.js

+
+  onMediaRecorderStop = () => {
+    // Chrome gives us video/webm even though we've requested audio
+    // for some reason. The data's still just audio though.


It might be more sensible if given a mimeType in the MediaRecorder constructor

Seems like Chrome only supports recording WebM, see https://stackoverflow.com/questions/41739837/all-mime-types-supported-by-mediarecorder-in-firefox-and-chrome and links in the answer. Is that important?

dbkr · 2017-02-16T18:03:14Z

src/components/views/rooms/AudioRecorder.js

+    const AccessibleButton = sdk.getComponent("elements.AccessibleButton");
+
+    const className = classNames('mx_MessageComposer_ptt', {
+      'mx_MessageComposer_ptt--recording': this.state.recording,


what's the -- syntax?

a habit I got from using getbem.com on some of my projects but will happily get rid of if it's an issue

aviraldg · 2017-02-18T13:31:22Z

Which is why this was commented out later.

…

On Saturday 18 February 2017 06:57 PM, Will Hunt wrote: ***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/components/views/messages/MAudioBody.js <#690 (comment)>: > return ( <span className="mx_MAudioBody"> - <audio src={contentUrl} controls /> + <audio src={contentUrl} controls autoPlay={shouldAutoPlay} /> This would also presumably play all files as you scrolled past them in the backlog Wouldn't this /also/ play sounds as they get rendered. So when I log in, or switch rooms? That sounds quite terrifying. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#690 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOlRIuQHBlf92kML9zcS9h4TISB9nPOks5rdvHMgaJpZM4L-K2z>.

-- Regards, Aviral Dasgupta

Half-Shot · 2017-02-18T13:36:27Z

@aviraldg Apologies, GH wasn't doing a very good job of hiding old code :/

dtygel · 2017-05-04T12:25:52Z

@aviraldg Have you stopped developing this feature?

aviraldg · 2017-05-04T14:03:25Z

The basic feature works as-is, but is blocked on speccing the allowed formats (something I haven't had time to do yet.) If you are interested in seeing this merged, discussing that with the team and proposing a change to the spec for it would be a good start.

On Thu, May 4, 2017 at 5:56 PM dtygel ***@***.***> wrote: @aviraldg <https://github.com/aviraldg> Have you stoppped developping this feature? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#690 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOlRCu8WACqo_-MnrTCLZ3KEADukHgyks5r2cPQgaJpZM4L-K2z> .

-- Regards, Aviral Dasgupta

superdump · 2017-08-29T11:49:59Z

As a note, the last time I checked (around the beginning of 2017), format support with the MediaRecorder API on different browsers varies significantly. Chrome would produce opus/webm and Firefox opus/ogg. I think there were issues with playback too as well as the MediaRecorder API implementations not always giving the errors they were supposed to meaning that you had to test, verify and tip-toe around how the browsers actually behaved. I would definitely expect the situation to have improved but I'm not certain of it. Testing would be needed. 😄

dtygel · 2017-08-29T15:15:11Z

What does telegram web and whatsapp web use for recording and reproducing audio? It seems to work without issues...

ghost · 2017-08-29T23:10:07Z

The basic feature works as-is, but is blocked on speccing the allowed formats (something I haven't had time to do yet.) If you are interested in seeing this merged, discussing that with the team and proposing a change to the spec for it would be a good start.

Where do you discuss things with the team? I did some research and suggest the Opus codec. It's state of the art and freely licensed. Chrome and Firefox support it, but Safari doesn't. Is that an issue? Chrome records WebM files and Firefox .ogg files, but both browsers can play both formats. What more information is required?

dtygel · 2017-09-02T03:57:23Z

@WorldCodeCentral , discussions on riot development happen in riot-dev room: https://riot.im/app/#/room/#riot-dev:matrix.org

jryans · 2019-07-31T11:02:59Z

Thanks for making this contribution a while back. Since the code base has changed since this was opened, it no longer applies cleanly, and I don't think there's a need to keep it open in this state, as we can always find the code here again if needed. If you are still interested in pursuing this feature, please discuss with us in #riot-dev:matrix.org to find a good approach forward.

aviraldg requested a review from dbkr February 11, 2017 13:04

aviraldg mentioned this pull request Feb 12, 2017

Voice message support matrix-org/matrix-spec-proposals#310

Closed

dbkr reviewed Feb 13, 2017

View reviewed changes

push-to-talk support

f85bf00

(fixes element-hq/element-web#1358)

aviraldg force-pushed the feature-pushtotalk branch from bded3f4 to f85bf00 Compare February 16, 2017 11:25

PTT: Use standard MediaRecorder instead of library

6dc231d

Also, some usability enhancements

aviraldg requested a review from dbkr February 16, 2017 14:29

dbkr suggested changes Feb 16, 2017

View reviewed changes

spacekitteh mentioned this pull request Apr 10, 2017

Button to record audio snippets and send them as audio events (voice messages) element-hq/element-web#1358

Closed

aviraldg mentioned this pull request Jun 2, 2017

styling for push to talk button element-hq/element-web#3240

Closed

jryans closed this Jul 31, 2019

Push to talk #690

Push to talk #690

Conversation

aviraldg commented Feb 11, 2017

matrixbot commented Feb 11, 2017

dbkr commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbkr commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviraldg commented Feb 18, 2017 via email

Half-Shot commented Feb 18, 2017

dtygel commented May 4, 2017 • edited Loading

aviraldg commented May 4, 2017 via email

superdump commented Aug 29, 2017

dtygel commented Aug 29, 2017

ghost commented Aug 29, 2017 • edited by ghost Loading

dtygel commented Sep 2, 2017

jryans commented Jul 31, 2019

dtygel commented May 4, 2017 •

edited

Loading

ghost commented Aug 29, 2017 •

edited by ghost

Loading