Fix Android TV icon when screencap option is disabled

[JeffLIrion]

Proposed change

If the screencap option is disabled, then the MediaPlayerEntity.media_image_hash should return None because an image was not fetched. This fixes an issue where the media player's icon is not displayed because it is overridden by a nonexistent entity picture.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

• This PR fixes or closes issue: fixes AndroidTV: media_player entity has no icon when device is turned on #35655
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[elupus]

While this isn't IO, wouldn't it be better to store timestamp in async_update() method? I'm mainly asking here to understand. As it is now you get different image hashes every call to this property. So from caching perspective this totally kills it.

[JeffLIrion]

While this isn't IO, wouldn't it be better to store timestamp in async_update() method? I'm mainly asking here to understand. As it is now you get different image hashes every call to this property. So from caching perspective this totally kills it.

I don't know. I just wanted to fix the issue that HA tries to display an entity picture when there isn't one (because the screencap option is off), and as a result the icon is not displayed.

[JeffLIrion]

@elupus to expand a bit on my previous response, the screencap functionality was introduced in #33232, and with it came the media_image_hash property. I'm not familiar with how HA handles fetching these images and caching, but looking at the MediaPlayerEntity.media_image_hash property it's clear that if there is no entity picture -- as is the case when the screencap option is disabled -- then that property should be None.

[JeffLIrion]

I’ve tested this change and it works.

Could this be cherry-picked for the next release?

[bdraco]

By returning the timestamp you are effectively disabling the caching.

Is there a way to tell when the image is updated?

[JeffLIrion]

By returning the timestamp you are effectively disabling the caching.

Is there a way to tell when the image is updated?

That's a question for @i00. The screencap functionality was introduced in #33232.

I'm not familiar with how HA handles the images that the integration retrieves, I just know that if you have the screencap option disabled then the media_image_hash property should return None.

[bdraco]

@JeffLIrion Would you please fill out what the Proposed change describing what this is fixing. I realize can figure it out from the linked issue, but thats not what we use when generating the release notes.

This also helps guide the reviewer as to what is changing here and give us an idea of the scope of the change.

Please remove the Breaking Change section

[JeffLIrion]

@bdraco done.

[bdraco]

We should look into getting rid of the timestamp here in a new PR as the cache exists for a reason.

Its outside of scope for a bugfix though.

[i00]

@bdraco ... how is the cache a good thing for something that continually changes?... There are at least 2 different media player integrations (I know of) that require no image caching... so maybe this should be able to be disabled within MediaPlayerEntity?

On another note, I have opened a frontend issue about the flickering (I use a custom media player card that does not flicker):
home-assistant/frontend#6067

[bdraco]

@bdraco ... how is the cache a good thing for something that continually changes?... There are at least 2 different media player integrations (I know of) that require no image caching... so maybe this should be able to be disabled within MediaPlayerEntity?

On another note, I have opened a frontend issue about the flickering (I use a custom media player card that does not flicker):
home-assistant/frontend#6067

It looks like system was originally designed to display album artwork in #5754 and the hash gets generated here: https://github.com/postlund/pyatv/blob/master/pyatv/interface.py#L443

When we send a timestamp instead of a hash here its abusing the original design (not a problem until it is). It does work as you are expecting sans the flickering from the original design not expecting the update frequency or image size.

We should probably change media_player to better handle integrations that don't want caching.

[i00]

We should probably change media_player to better handle integrations that don't want caching.

As I said 👍

It does work as you are expecting sans the flickering from the original design not expecting the update frequency or image size.

Yes ... but this also occurs when the song advances and still doesn't look that pleasant when this does occur; and should be fixed regardless in the media cards; and on the "circular" entity previews.