Improve performance of placeholder images #1325
Conversation
…is will allow browers to cache the place holder
|
It appears the API does not make use of the "updatedAt" field so I believe it's removal should have no negative side effects. navidrome/server/subsonic/media_retrieval.go Lines 60 to 63 in af7c87d |
core/artwork.go
Outdated
| @@ -127,6 +128,12 @@ func (a *artwork) getArtwork(ctx context.Context, id string, path string, size i | |||
| if err != nil { | |||
| log.Warn(ctx, "Error extracting image", "path", path, "size", size, err) | |||
| reader, err = resources.FS.Open(consts.PlaceholderAlbumArt) | |||
|
|
|||
| if size != 0 { | |||
There was a problem hiding this comment.
You should also check for err == nil here
There was a problem hiding this comment.
You mean like so?
if (size != 0 && err == nil)There was a problem hiding this comment.
Yes. if it returns error, you should not call resizeImage with the streamer (it may even be nil)
| } | ||
|
|
||
| return | ||
| } | ||
|
|
||
| func resizeImage(reader io.Reader, size int) (io.ReadCloser, error) { | ||
| func resizeImage(reader io.Reader, size int, usePng bool) (io.ReadCloser, error) { |
There was a problem hiding this comment.
For the transparency. It threw off the visual style when it suddenly got a white background.
ui/src/subsonic/index.js
Outdated
| @@ -49,7 +49,9 @@ const getCoverArtUrl = (record, size) => { | |||
| ...(record.updatedAt && { _: record.updatedAt }), | |||
| ...(size && { size }), | |||
| } | |||
| return baseUrl(url('getCoverArt', record.coverArtId || 'not_found', options)) | |||
| if (record.coverArtId) | |||
There was a problem hiding this comment.
Just a bit of nitpick: Can you put braces {} around the statements in the if and else blocks? Or rewrite it with a ternary operator:
return record.coverArtId
? baseUrl(url('getCoverArt', record.coverArtId, options))
: baseUrl(url('getCoverArt', 'not_found', size && { size }))I strive to avoid if blocks without braces throughout the code. This arguably makes code easier to maintain, reducing the possibility of wanting to add a new statement in the if block and forgetting to add the braces. This is similar to what Go forces you to do: https://golang.org/doc/effective_go#if
There was a problem hiding this comment.
No problem. Opt'd for braces in this case. The ternary seems a bit long and unweildy
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR improves performance for placeholder images:
I'm leaving this as a draft for now as I don't know if there are negative impacts from removing the
updatedAtfield from the getCoverArt requests for placeholders.Original behavior
Original behavior has to make a request for each placeholder image at original size. My system/internet has lousy upload performance resulting in noticeable delay in fetching placeholders.
New behavior
With the following changes only a single request for the place holder is performed, and at a reduced size.