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

MSC3245: Voice messages (using extensible events) #3245

Open
wants to merge 13 commits into
base: old_master
Choose a base branch
from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jun 15, 2021

@turt2live turt2live changed the title MSC0000: Voice messages (using extensible events) MSC3245: Voice messages (using extensible events) Jun 15, 2021
@turt2live turt2live added client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal proposal-in-review labels Jun 15, 2021
@turt2live turt2live marked this pull request as ready for review June 15, 2021 22:57
proposals/3245-voice-messages.md Outdated Show resolved Hide resolved
proposals/3245-voice-messages.md Show resolved Hide resolved
@turt2live
Copy link
Member Author

This hasn't really received much negative press, and has been working well in Element Web for a while. The mobile apps appear to be handling the Opus/Ogg bits well enough. All 3 test platforms ran into issues with encoding Opus, but all appear to have jumped that hurdle with relative ease.

As such, a plea for final comments:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Jul 14, 2021

This FCP proposal has been cancelled by #3245 (comment).

Team member @mscbot has proposed to merge this. The next step is review by the rest of the tagged people:

Concerns:

  • Too much of a reliance on extensible events to move forward

Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for information about what commands tagged team members can give me.

@mscbot mscbot added disposition-merge proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. and removed proposal-in-review labels Jul 14, 2021
Copy link

@kevincox kevincox left a comment

Choose a reason for hiding this comment

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

Sorry, forgot to click send.

proposals/3245-voice-messages.md Show resolved Hide resolved
proposals/3245-voice-messages.md Outdated Show resolved Hide resolved
Co-authored-by: David Baker <dbkr@users.noreply.github.com>
proposals/3245-voice-messages.md Outdated Show resolved Hide resolved
Comment on lines 77 to 82
The `m.voice` identifier could probably conflict, as `m.audio` could conflict as well. We may be interested in
discussing `m.message.voice` or similar instead, though likely at MSC1767 rather than this proposal.

This also annotates events with an empty object and potentially a lot of extra information. This is considered
to be an issue for MSC1767 to consider rather than this MSC, as this MSC is simply saying to create audio events
with some sort of voice message type.
Copy link
Member

Choose a reason for hiding this comment

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

it seems like we need to resolve these issues in MSC1767 before we can sensibly complete this MSC.

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@turt2live
Copy link
Member Author

@mscbot concern Too much of a reliance on extensible events to move forward

#3245 (comment)

@mscbot mscbot added the unresolved-concerns This proposal has at least one outstanding concern label Jul 27, 2021
Co-authored-by: Hubert Chathi <hubert@uhoreg.ca>
@turt2live
Copy link
Member Author

This is blocked and unable to make forward progress, so pulling it out of the SCT's "look at this now" queue:

@mscbot fcp cancel

Comment on lines +3 to +5
Voice messages are a useful way to quickly send a message to someone without having to use the more
awkward keyboard. Typically short in length, voice messages can be sent as annotated audio files
to recipients.
Copy link
Contributor

Choose a reason for hiding this comment

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

This MSC doesn't motivate why voice messages should exist as a separate concept in addition to the more generic audio messages. I had to go over to MSC2516 to find some (potential) reasons. Could that be added?

@jonnyandrew

This comment was marked as duplicate.

trusted, and the user's voice is not uploaded plainly to the media repo. Typically, this will be DMs or other
forms of private chats in most clients.

## Unstable prefix
Copy link
Member Author

Choose a reason for hiding this comment

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

@jonnyandrew says:

@turt2live we're planning to implement this MSC in Element X but we're conscious that an older version (v1) has already been implemented in the Element Classic apps. Should we prefer to implement v2 in Element X in order to help progress the MSC? Or will it be okay to stick with v1 so that EC and EX can talk to each other? cc @nimau

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't recall which version is already deployed out in the wild, but would recommend using the one that other clients use. If it turns out that no one implements the v2 stuff (like in the case with Polls), we can fix this MSC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff proposal A matrix spec change proposal unresolved-concerns This proposal has at least one outstanding concern
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants