Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Expand upon voice message event & include overall waveform #5888

Merged
merged 6 commits into from
Apr 22, 2021

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Apr 20, 2021

This does two primary things:

  1. Gets rid of the deprecated processor usage, replacing things with a worklet.
  2. Determines the final waveform of the recorded clip to include it in the event. This will later be used for timeline rendering.

There's no specification or MSC for what the waveform attribute would contain, so this assumes that "one sample per second" is suitable. This is highly likely to change as a 1 second clip is likely to need a few dozen more samples.

Note that this mixes in a couple MSCs to the event format. This is part of an experiment to see how it ends up working out.


Requires element-hq/element-web#17013


@turt2live turt2live requested a review from a team April 20, 2021 05:12
throw new Error("Unable to create recorder: no worklet script registered");
}
await this.recorderContext.audioWorklet.addModule(mxRecorderWorkletPath);
this.recorderWorklet = new AudioWorkletNode(this.recorderContext, WORKLET_NAME);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the AudioWorkletNode is not supported by Safari whereas the ScriptProcessorNode was.
Is there a fallback or progressive enhancement setup somewhere that I might not be aware of?

Copy link
Member Author

Choose a reason for hiding this comment

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

ugh, I'll make note of this and get it properly tested in Safari in a future PR. I'm not confident it works properly before this change anyways.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll definitely need some plan to support Safari before this can be enabled by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

plan is in progress, though is dependent on acquisition of a Mac (work in progress)

Copy link
Contributor

@germain-gg germain-gg left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👏
I'll leave the final review to someone else in the team

@germain-gg germain-gg requested a review from a team April 20, 2021 07:48
Copy link
Collaborator

@jryans jryans left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me, but please do consider the impact on Safari, as we'll need it to work there as before the feature can be enabled.

src/@types/global.d.ts Outdated Show resolved Hide resolved
src/voice/RecorderWorklet.ts Outdated Show resolved Hide resolved
throw new Error("Unable to create recorder: no worklet script registered");
}
await this.recorderContext.audioWorklet.addModule(mxRecorderWorkletPath);
this.recorderWorklet = new AudioWorkletNode(this.recorderContext, WORKLET_NAME);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We'll definitely need some plan to support Safari before this can be enabled by default.

@turt2live turt2live merged commit 06726d3 into develop Apr 22, 2021
@turt2live turt2live deleted the travis/voice/event_type branch April 22, 2021 20:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants