Add media_image to media_player component

[postlund]

Description:
Purpose of this PR is to add a media_image concept to media_player that can serve binary images (when a platform provides that instead of a URL) through media_player_proxy. This is based on comments from @balloob in PR #5698.

It is currently not complete as the frontend must somehow know if there is an image available. This is determined by media_entity_picture and that doesn't really fit with this implementation. Ideas to solve this is welcome as that would help me complete the Apple TV platform.

Checklist:

If the code does not interact with devices:

• Local tests with tox run successfully. Your PR cannot be merged unless tests pass
• Tests have been added to verify that the new code works.

[mention-bot]

@postlund, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabaff and @armills to be potential reviewers.

[pvizeli]

I think that will kill the new cache layer on media_player and give a bad performance while it fetch this ever state change for "platform" fetch. I known that is only for platform they use this way.

I think it will be better to have it as property and update with platform update process. So the platform can update if it needed with regular update interval. I think that will a lot of more perform as a function they will call ever state change.

[postlund]

I agree and that is how I have implemented it in the Apple TV platform (but I have not pushed that yet). In this case, the async version of the get method is not really needed I guess (since we already have the image cached in the platform). But this still doesn't solve the problem of frontend not knowing that an image exist. Since all requests to the proxy are redirected if a cache image exist, any url from media_image_url will do. So I guess a hack would be to just return "something" in that method...

[andrey-git]

I agree with @pvizeli
The property that would store the image itself (not the url)

However this has a downside of the platform fetching the image even if no one is looking at the UI.

The platform can do something complicated, like each time the property is queried save a timestamp that will cause the it to update for some time in the future.

As far as frontend is concerned it looks at the existence of entity_picture property, which is by default computed from media_image_url.
If entity_picture is changed to look at both media_image_url and the new property - no frontend changes will be needed.

[postlund]

I agree @andrey-git, it's not optimal that the image is fetched even when the frontend is not used. But I still think it's fine, changing the API in pyatv to return a URL would be more of a hack than this. I can probably re-work this in a better way later.

[andrey-git]

media image could be (None, None).

[postlund]

Automatically fixed by my new commit.

[andrey-git]

A constant url will lead to constant md5 and the browser caching this image.

The md5 should run on the image itself, or part of it or a separate property.

[postlund]

Good point. I added so that media_image also returns a hash. This way the hash only needs to be calculated once instead of with every call.

[andrey-git]

@pvizeli You OK with merging this?

[pvizeli]

Need only (None, None) since it will evertime create a hash on https://github.com/postlund/home-assistant/blob/654ba940dc8d93e02445bf8fcc2255895a90472c/homeassistant/components/media_player/__init__.py#L826-L828 so we have no improve with that.

[postlund]

Problem is that the hash generated by entity_picture depends on url - all the other factors are always the same (entity_id and access token). So in reality, the hash is just a transformation of the url from an entity point-of-view, so it must be different for every image otherwise we will always generate the same url for the client. This is basically what @andrey-git commented on earlier, so I still feel certain that we need to do this to avoid caching problems. Agree?

[pvizeli]

You have if url is None twice in code. make only else: return

[balloob]

I think that we should alter the code slightly. Properties are not allowed to do I/O so it means that whenever we call update, platforms implementing media_image will have to download the album cover. That is not optimal.

Another tricky part is the hashing part of images. We can't know if the image has changed until we get the image. I think because of that we should add a hash property media_image_hash. This will solve the entity_picture problem.

Can I suggest a slightly different approach:

• Add a media_image_hash property that returns a hash string, defaults to None
• Test if media player supports media image by checking if player.media_image_hash is not None
• Add async_get_media_image method (with sync version get_media_image) that will return bytes and content type.

absoluted

[postlund]

Good point. I will try to update according to this tonight.

[jjmontesl]

Hi, according to gitter chat, I'd like to suggest to use a more global solution. URLs might allow for file:// schemas. This would be extensible later to other schemas (ftp, memory-mapped...). Caching might be included here too.

I have implemented file:// support in media_player (jjmontesl@9e28138) which might be of interest, but is still limited to media_player. I suggest a more global solution to reduce code.

[postlund]

@balloob I've tried to get my head around this but I don't think I fully understand how it's supposed to work. How does player.media_image_hash and async_get_media_image/get_media_image fit together? I can do an estimated guess that an image has changed when and update is performed (but in practice I can't know for sure as there are no unique identifiers involved).

[balloob]

Something like this: (note: didn't run this code)

    # Overwrite this in ATV case
    @property
    def media_image_hash(self):
        url = self.media_image_url

        if url is not None:
            return hashlib.md5(url.encode('utf-8')).hexdigest()[:5]

        return None

    # Overwrite this in ATV case
    @asyncio.coroutine
    def async_get_media_image(self):
        url = self.media_image_url
        if url is None:
            return None, None

        return (yield from _async_fetch_image(self.hass, url))

    @property
    def entity_picture(self):
        """Return image of the media playing."""
        if self.state == STATE_OFF:
            return None

        image_hash = self.media_image_hash

        if image_hash is None:
            return None

        return ENTITY_IMAGE_URL.format(
            self.entity_id, self.access_token, image_hash)

And for the web view:

    @asyncio.coroutine
    def get(self, request, entity_id):
        """Start a get request."""
        player = self.entities.get(entity_id)
        if player is None:
            status = 404 if request[KEY_AUTHENTICATED] else 401
            return web.Response(status=status)

        authenticated = (request[KEY_AUTHENTICATED] or
                         request.GET.get('token') == player.access_token)

        if not authenticated:
            return web.Response(status=401)

        data, content_type = yield from player.async_get_media_image()

        if data is None:
            return web.Response(status=500)

        return web.Response(body=data, content_type=content_type)

[postlund]

Ok, looks good. I guess I have to check if get_media_image exists in the view and run that in executor if that's the case? Also, this still doesn't really fit the pattern. As I said, I can make an educated guess during update but I can't generate a valid hash until I have the image. So it becomes a circular dependency. A hack , I guess, would be to generate a new hash based on something every time the image might have changed. But now, I'm beginning to consider adding URL support pyatv. That would make life a lot easier.

[balloob]

You don't have to generate the hash based on the image content. You can base it on the content ID, current artist + album + song title or whatever information you have available to uniquely identify the content.

[postlund]

You are so wise, @balloob! 😄 I made your proposed changes and they work. Will update my other PR with the Apple TV changes. Then we should be able to leave this behind.

[jjmontesl]

Please update this when you are done, as #5877 is waiting for this :-)

[balloob]

Great code! 😉 🐬

[pvizeli]

Unittests?