-
Notifications
You must be signed in to change notification settings - Fork 424
[WIP] MSC4396: Inline linked media #4396
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
base: main
Are you sure you want to change the base?
Conversation
| Combining the operations ensures there's a guaranteed link between media and event, but does also | ||
| mean that media must be available to be combined in the operation. This also may lead to client |
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.
How do you intend to handle forwarding of events? Is media just re-uploaded?
Edits pose a similar problem, but that can be solved by media being attached to the original event at least. State events are slightly more problematic.
| **Note**: The server *always* overwrites `m.media`, even if there's no media to assign to the event. | ||
| If the `m.media` array is empty, the field SHOULD be deleted from the `content` instead. | ||
|
|
||
| `m.media` is an array of linked (or assigned, or attached, or $synonym) media URIs, like so: |
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.
Just to understand this correctly, how would I handle the case when I want to add an additional file to a state event? Do I have to reupload all media files or does the server merge the list of media files. It reads like the former, but that seems super wasteful?
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.
Events should only contain a single logical datum, so there shouldn't be multiple files in a single event (state or otherwise).
|
Can you add a section for Motivation/why you want this? It looks to me like it would badly break backwards compatibility, and I'm not sure why this is needed (if the positives outweigh the drawbacks). So that I'm not missing anything, it would help if you directly explained the advantages that this MSC would provide. |
|
@cloudrac3r top level comments on MSCs are often ignored/missed - please use the diff instead for future comments. As this is a work in progress MSC, it is not expected to have all of the sections or details. The motivation is partially mentioned in the introduction already, and MSC3911 has a bit more direct detail. |
| 2. `X-Matrix-Request-Async-File` is a header on the third part. By setting this to `true`, the caller | ||
| is saying that it doesn't yet have the media and would like to use the (newly defined) | ||
| `PUT /_matrix/client/v1/media/upload/:serverName/:mediaId` endpoint instead. The caller discovers | ||
| the media ID to upload to via the endpoint's response. |
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.
I think it's not necessarily that the caller doesn't yet have the media but more that it wants to upload it asynchronously so that the event itself can be sent and read by recipients without blocking on the upload?
|
|
||
| The general concept of MSC3911 is preserved by this proposal: media *must* be linked to an event. The | ||
| difference is how that linking happens. In MSC3911, media is "attached" to an event at send-time after | ||
| being uploaded. In this proposal, media is uploaded at send-time and attached in the same operation. |
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.
I found "media is uploaded at send-time" slightly misleading because the proposal still allows async uploads.
| Users receive an `403 M_FORBIDDEN` error when downloading media associated with an event they cannot | ||
| see. |
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.
Should this also optionally allow 404 as in the removal case above?
| scan it, then take relevant action. With the combined operation, the safety tool can skip the | ||
| extraction and download steps, improving response time. | ||
|
|
||
| **TODO**: More? |
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.
It seems like the proposal only partially addresses the single listed use case because async uploads still exist. So more use cases would really be helpful here, in my opinion.
| Profile changes often result in `m.room.member` events being sent. This proposal changes the | ||
| [`PUT /_matrix/client/v3/profile/:userId/:keyName`](https://spec.matrix.org/v1.17/client-server-api/#put_matrixclientv3profileuseridkeyname) | ||
| endpoint to support the new `m.media` mixin, but requires special handling because the data is not | ||
| in a traditional event shape. When `keyName` is `avatar_url`, the multipart request body schema still | ||
| applies, though the JSON object part will always be an empty object. The first (and hopefully, only) | ||
| media part will become the user's avatar, populating both the `avatar_url` and `m.media` fields on | ||
| the resulting `m.room.member` event. |
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.
I think a request example might help to better grasp this. It would look like this, if I understood correctly?
PUT /_matrix/client/v3/profile/{userId}/avatar_url
Authorization: Bearer TOKEN
Content-Type: multipart/mixed; boundary=a1b2c3
--a1b2c3
Content-Type: application/json
{}
--a1b2c3
Content-Type: image/png
[png data]
I assume async uploads are not allowed here?
Does the server echo back media_uris in the response?
Rendered
Disclosure: I am Director of Standards Development at The Matrix.org Foundation C.I.C., Matrix Spec Core Team (SCT) member, employed by Element, and operate the t2bot.io service. This proposal is written and published as a Trust & Safety team member allocated in full to the Foundation.