Skip to content

Don't proxy external accessible covers#3120

Merged
balloob merged 1 commit intodevfrom
external-media-player
Apr 25, 2019
Merged

Don't proxy external accessible covers#3120
balloob merged 1 commit intodevfrom
external-media-player

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented Apr 24, 2019

Requires home-assistant/core#23337

When a media player has an external accessible cover, don't proxy the data through Home Assistant backend.

@thomasloven
Copy link
Copy Markdown
Contributor

I'm not really up to date on how the media_player domain works, but what if the cover is served from a local network address? Maybe that's not even possible, in which case it's obviously not a problem?

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Apr 24, 2019

In that case the cover is not externally accessible and we fall back to the current behavior, which is to proxy it. See also the linked backend PR.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Apr 24, 2019

Right, what is causing confusion here is that entity_picture is a relative link to the proxy url when the picture is not remotely accessible.

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Apr 24, 2019

So an entity_picture is either https://www.example.com/cover.png or it's /api/media_player_proxy/media_player.kitchen


// We have a new picture url
// If entity picture is non-relative, we use that url directly.
if (picture.substr(0, 1) !== "/") {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This seems kinda smelly... would it be better to add media_image_remotely_accessible to state attributes so we can key off that directly?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree that it is a little fragile. This implementation is directly tied to how the media player is implemented. If album art is not externally accessible, we set it to a relative url that points at the media player album art proxy. (which sucks btw, because it means we change states every 30s for no good reason)

@balloob
Copy link
Copy Markdown
Member Author

balloob commented Apr 25, 2019

I have been thinking, adding a property is a bit weird too. Let's just ship it and 🚀

@balloob balloob merged commit cd6250c into dev Apr 25, 2019
@delete-merged-branch delete-merged-branch Bot deleted the external-media-player branch April 25, 2019 04:05
@balloob balloob mentioned this pull request Apr 28, 2019
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants