-
Notifications
You must be signed in to change notification settings - Fork 28
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
Imagemagickを抹殺 #2143
Imagemagickを抹殺 #2143
Conversation
c3873b0
to
4b6339b
Compare
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.
細かいライブラリの使い方までは見てないですが、動作確認が取れてれば良いと思います
(実際に目で確認して、画像サイズ・ファイルサイズが縮小できてるか、縦横比があってるかどうか それをテストでコード化できれば最高)
確認してほしいコメントをいくつかしました
gif周りのテストが変更後は存在しなくなってるように見えるので、できればPRに上げてくれた画像などで追加したいですね
そんなに掛かるんだ... PRの検証を見たところ、imagemagickとそう大差は無いようですが、気に留めておきたいかもですね |
Co-authored-by: motoki317 <motoki317@gmail.com>
フレーム数だけリサイズをしなければいけないので、それだけかかるのは仕方なそうな感じがあります |
Co-authored-by: motoki317 <motoki317@gmail.com>
メモリ使用量はチェックしておくと良さそうです |
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.
頑張って並列化してくれたのは面白いけど、本番で必ずしもコア数が多いマシンや多くのCPUが許可されたコンテナで動くとは限らないので、計測時ほど差は出ないかもしれないです
This reverts commit 2185fc8.
circular pointer reference
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.
テスト書いてあるしまあ良いかな...(複雑なのはちょっと気になるけど)
あとこれは別PRにしてほしいけど、ファイルアップロードとアイコンアップロード周りでごちゃっとしてたり重複する処理が書かれてたりするから、リファクタリングしたさがある
utils/utils.go
Outdated
func Map[T, R any](s []T, mapper func(item T) R) []R { | ||
ret := make([]R, len(s)) | ||
for i := range s { | ||
ret[i] = mapper(s[i]) | ||
} | ||
return ret | ||
} | ||
|
||
func IoReaderToBytes(r io.Reader) ([]byte, error) { |
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.
io.ReadAllが同値かも
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.
ReadAllって使って良かったっけってなっちゃったけど、よく考えたらそれioutilの話か…ってなりました
書き換えます
utils/utils.go
Outdated
return buf.Bytes(), nil | ||
} | ||
|
||
func MustIoReaderToBytes(r io.Reader) []byte { |
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.
lo.Mustが便利かも
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.
初耳だった
見ます
これやると #1875 とかもやりやすくなるのかなって少し思いました |
それはアップロード処理とは関係無いような |
なるほど、何も見ずとりあえず言ってみただけなので多分そっちが正しい |
背景
実装