schemas/account,account_view: add akkoma.media.*_preview #1071

Open
Yonle wants to merge 1 commit from Yonle/akkoma:profmedprev-1 into develop
Contributor
as discussed on https://akkoma.dev/AkkomaGang/akkoma-fe/issues/481#issuecomment-16075 for https://akkoma.dev/AkkomaGang/akkoma/issues/1066
schemas/account,account_view: add akkoma.media.*_preview
Some checks are pending
ci/woodpecker/pr/test/1 Pipeline is pending approval
ci/woodpecker/pr/test/2 Pipeline is pending approval
c32029194a
Signed-off-by: Yonle <yonle@proton.me>
Oneric left a comment
Owner

One nit regarding the schema and you’ll also need to add the new fields to docs ("Differences from Masto API") and explain there how the fields differ from the plain and _static versions.

As discussed in the other issue, you’ll probably want to generate smaller images (in terms of dimensions and transfer size) for avatar previews than for regular image attachment thumbnails (and iinm the proxy cannot encode parameters into the (signed) path, meaning a new endpoint or the ability to encode params would be needed).
Though I guess the default of max 600px in any direction is already enough to curb the worst offenders?

Generally we don’t accept backend features without any client users. But you already have a FE PR to make use of it, so if both end up good enough for merge this shouldn’t become an issue.

One nit regarding the schema and you’ll also need to add the new fields to docs ("Differences from Masto API") and explain there how the fields differ from the plain and `_static` versions. As discussed in the other issue, you’ll probably want to generate _smaller_ images (in terms of dimensions and transfer size) for avatar previews than for regular image attachment thumbnails (and iinm the proxy cannot encode parameters into the (signed) path, meaning a new endpoint or the ability to encode params would be needed). Though I guess the default of max 600px in any direction is already enough to curb the worst offenders? Generally we don’t accept backend features without any client users. But you already have a FE PR to make use of it, so if both end up good enough for merge this shouldn’t become an issue.
@ -127,0 +131,4 @@
avatar_preview: %Schema{type: :string, format: :uri},
header_preview: %Schema{type: :string, format: :uri}
}
}
Owner

I don’t think a media subobject inside the akkoma subobject is necessary. You can just put it directly into akkoma

I don’t think a `media` subobject inside the `akkoma` subobject is necessary. You can just put it directly into `akkoma`
Author
Contributor

@Oneric wrote in #1071 (comment):

you’ll probably want to generate smaller images (in terms of dimensions and transfer size) for avatar previews than for regular image attachment thumbnails

it will be up to the mediaproxy to do that.

meaning a new endpoint or the ability to encode params would be needed).
Though I guess the default of max 600px in any direction is already enough to curb the worst offenders

i would say the default size of a thumbnailed ones are already perfect.

however, before we even begin with this, there's actually a major problem on the thumbnailer itself where it can't convert animated avatar/banner to a compressed format. I have a custom mediaproxy which did exactly that, however this thing make me stuck for quite a while.

@Oneric wrote in https://akkoma.dev/AkkomaGang/akkoma/pulls/1071#issuecomment-16537: > you’ll probably want to generate _smaller_ images (in terms of dimensions and transfer size) for avatar previews than for regular image attachment thumbnails it will be up to the mediaproxy to do that. > meaning a new endpoint or the ability to encode params would be needed). > Though I guess the default of max 600px in any direction is already enough to curb the worst offenders i would say the default size of a thumbnailed ones are already perfect. however, before we even begin with this, there's actually a major problem on the thumbnailer itself where it can't convert animated avatar/banner to a compressed format. I have a [custom mediaproxy](https://github.com/Yonle/mediaproxyoma) which did exactly that, however this thing make me stuck for quite a while.
Owner

it will be up to the mediaproxy to do that.

The proxy currently has no idea though whether the preview URL is for an avatar or a full attachment thumbnail atm, as described in prev post. And without knowing it can’t take care of it
If you’re thinking of query url parameters, they are not signed as far as I understand and thus susceptible to being replaced with much larger dimensions by anyone

> it will be up to the mediaproxy to do that. The proxy currently has no idea though whether the preview URL is for an avatar or a full attachment thumbnail atm, as described in prev post. And without knowing it can’t take care of it If you’re thinking of query url parameters, they are not signed as far as I understand and thus susceptible to being replaced with much larger dimensions by anyone
Owner

there's actually a major problem on the thumbnailer itself where it can't convert animated avatar/banner to a compressed format.

Can you clarify whether this is just an "it does not avoid as much network traffic as it could" issue, or does it make enablng previews somehow worse than no previews at all? If it’s the former, exacerbating the issue with more preview links seems indeed unfortunately ill-advised. But if it’s the latter this shouldn’t be a blocker for merging this (and you just need to adjust the schema as per previous review comments)

Either way you’re ofc welcome to improve the built-in proxy (though then someone more familiar with its everyday workings would need to pick up reviewing), but if you already have an out-of-tree solution which works for you you presumably don’t want to spend time on something without return.

> there's actually a major problem on the thumbnailer itself where it can't convert animated avatar/banner to a compressed format. Can you clarify whether this is just an "it does not avoid as much network traffic as it could" issue, or does it make enablng previews somehow _worse_ than no previews at all? If it’s the former, exacerbating the issue with more preview links seems indeed unfortunately ill-advised. But if it’s the latter this shouldn’t be a blocker for merging this *(and you just need to adjust the schema as per previous review comments)* Either way you’re ofc welcome to improve the built-in proxy *(though then someone more familiar with its everyday workings would need to pick up reviewing)*, but if you already have an out-of-tree solution which works for you you presumably don’t want to spend time on something without return.
Author
Contributor

@Oneric wrote in #1071 (comment):

Can you clarify whether this is just an "it does not avoid as much network traffic as it could" issue, or does it make enablng previews somehow worse than no previews at all?

at this current condition, it's not really worse, but did not really solve the main issue, which you can check from the following, that it's redirecting and did only proxying:

content_type == "image/gif" ->
redirect(conn, external: media_proxy_url)

@Oneric wrote in #1071 (comment):

but if you already have an out-of-tree solution which works for you you presumably don’t want to spend time on something without return

it's kinda rough as elixir itself is not my main programming language (my main lang is go), but probably making a separate issue on fixing the media proxy might also work? not really sure at this point or probably keeping the current flow only on my end

@Oneric wrote in https://akkoma.dev/AkkomaGang/akkoma/pulls/1071#issuecomment-16629: > Can you clarify whether this is just an "it does not avoid as much network traffic as it could" issue, or does it make enablng previews somehow _worse_ than no previews at all? at this current condition, it's not really worse, but did not really solve the main issue, which you can check from the following, that it's redirecting and did only proxying: https://akkoma.dev/AkkomaGang/akkoma/src/commit/ea9d1f120511af052c109e8e9503f07944c56656/lib/pleroma/web/media_proxy/media_proxy_controller.ex#L70-L71@Oneric wrote in https://akkoma.dev/AkkomaGang/akkoma/pulls/1071#issuecomment-16629: > but if you already have an out-of-tree solution which works for you you presumably don’t want to spend time on something without return it's kinda rough as elixir itself is not my main programming language (my main lang is go), but probably making a separate issue on fixing the media proxy might also work? not really sure at this point or probably keeping the current flow only on my end
Owner

at this current condition, it's not really worse,

Thanks for clarifying. Then it’s no blocker for adding avatar and header previews and can be left for some future issue or patch

> at this current condition, it's not really worse, Thanks for clarifying. Then it’s no blocker for adding avatar and header previews and can be left for some future issue or patch
Some checks are pending
ci/woodpecker/pr/test/1 Pipeline is pending approval
ci/woodpecker/pr/test/2 Pipeline is pending approval
This pull request has changes conflicting with the target branch.
  • lib/pleroma/web/mastodon_api/views/account_view.ex
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u profmedprev-1:Yonle-profmedprev-1
git switch Yonle-profmedprev-1
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
AkkomaGang/akkoma!1071
No description provided.