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

MSC2700: Thumbnail requirements for the media repo #2700

Closed
wants to merge 3 commits into from

Conversation

turt2live
Copy link
Member

@turt2live turt2live commented Jul 28, 2020

@turt2live turt2live added kind:maintenance MSC which clarifies/updates existing spec proposal A matrix spec change proposal proposal-in-review labels Jul 28, 2020
@turt2live turt2live changed the title MSC0000: Thumbnail requirements for the media repo MSC2700: Thumbnail requirements for the media repo Jul 28, 2020
* `video/H264`
* `video/mp4`

Servers SHOULD support other mimetypes where possible, such as `application/pdf` and `text/plain`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify: this means that a media repository can still thumbnail any other formats that they want to?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, the minimum is provided above though

Copy link
Member

Choose a reason for hiding this comment

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

The open-ended list means that clients still cannot reliably predict which MIME types a given server can thumbnail, beyond a very limited set.
Can't we just rely on what comes from /sync or /event and move the decision on whether a file has thumbnail or not to the moment of uploading? Basically, the uploading client could instruct the server to automatically generate thumbnail (on top of supplying the mxc: link to the thumbnail, as it's done now) and get a respective message on whether or not this is possible in the response. Downloading clients would just use the event payloads as they can do now to find thumbnail information - except that if there's none, clients SHOULD NOT ask the server for a thumbnail.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thumbnail endpoint exists so clients can populate that information in the event. It's not always possible to do the thumbnailing client side (encrypted media shows the artifacts nicely,), and the client rendering the event might have different resolution requirements anyways.

tbh, another msc should remove the thumbnail info from the event as it provides much less value to clients.

The open-ended support is a requirement, however there's zero reason why we can't specify a limited set for clients to always try for.

Copy link
Member

Choose a reason for hiding this comment

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

The point about different resolutions is quite convincing, thanks for the reminder. But I still struggle to work out a coherent guidance for myself to implement this in Quaternion. Right now I just try to get a thumbnail for any message event with m.image or m.video. This MSC is supposed to change that but how exactly - this escapes me. I certainly don't want to restrain myself to the three required MIME types, but since the list is open-ended, I should - keep doing what I do now in the hope that the server supports more than just basics?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, as mentioned: the MSC needs rework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps an endpoint could be added where the server reports which mimetypes it can thumbnail? The client could query that on the first time it wants to thumbnail something and then make a decission based on on that

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We introduced https://matrix.org/docs/spec/client_server/r0.6.0#get-matrix-media-r0-config to be a general key value map of configuration to inform the client before they try a media operation. It feels like including a list of supported mimetypes would be trivial from a spec and impl perspective?

Copy link
Member

Choose a reason for hiding this comment

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

That would work, yes.

Servers SHOULD be able to generate thumbnails for the following mimetypes:
* `image/svg+xml`
* `video/H264`
* `video/mp4`
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also add image/webp to SHOULD as it is becoming more and more standard?

Copy link
Member Author

Choose a reason for hiding this comment

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

There doesn't really appear to be a lot of traction around it, at least not enough to warrant an explicit suggestion in the spec.

Copy link
Member

@KitsuneRal KitsuneRal left a comment

Choose a reason for hiding this comment

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

To be honest, I'm not sure what's proposed here has much merit from the client perspective. The basic list is too limited, the full list is open-ended - the MSC does not seem to fulfil its purpose.

Servers SHOULD support other mimetypes where possible, such as `application/pdf` and `text/plain`.

In order to better support the server's mission in thumbnailing, clients SHOULD NOT upload
encrypted with a content type that matches the source material. Instead, clients SHOULD use
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
encrypted with a content type that matches the source material. Instead, clients SHOULD use
encrypted files with a content type that matches the source material. Instead, clients SHOULD use

proposals/2700-thumbnail-types.md Show resolved Hide resolved
* `video/H264`
* `video/mp4`

Servers SHOULD support other mimetypes where possible, such as `application/pdf` and `text/plain`.
Copy link
Member

Choose a reason for hiding this comment

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

The open-ended list means that clients still cannot reliably predict which MIME types a given server can thumbnail, beyond a very limited set.
Can't we just rely on what comes from /sync or /event and move the decision on whether a file has thumbnail or not to the moment of uploading? Basically, the uploading client could instruct the server to automatically generate thumbnail (on top of supplying the mxc: link to the thumbnail, as it's done now) and get a respective message on whether or not this is possible in the response. Downloading clients would just use the event payloads as they can do now to find thumbnail information - except that if there's none, clients SHOULD NOT ask the server for a thumbnail.

@turt2live
Copy link
Member Author

It's possible like the other thumbnail MSC I wrote that this one is far too split apart to see the actual goal.

Currently what can be thumbnailed is left as an implementation concern and not one that can be
predicted or anticipated by clients. This relatively small proposal aims to fix that.

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

Thoughts about trying to predict supported mimetypes: Can we add a key to /config which includes which mimetypes are supported, much like we do with the max upload size today?

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'd rather not tbh, as it means we're in capabilities negotiation territory and less in the realm of spec.

@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021

## Security considerations

Some media, such as SVGs, are vulnerable to various well-known attacks. These should be avoided by
Copy link
Member

Choose a reason for hiding this comment

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

These should be avoided by server implementations.

Does "These" refer to the media types or to the attacks?

@turt2live
Copy link
Member Author

MSC4011 is a much better approach than a list of MUSTs and SHOULDs - closing this MSC in favour of that one.

@turt2live turt2live closed this Dec 16, 2023
@turt2live turt2live added the obsolete A proposal which has been overtaken by other proposals label Dec 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants