Avoid unnecessary cast state updates by dersger · Pull Request #13770 · home-assistant/core

dersger · 2018-04-08T20:45:32Z

Description:

Got a lot of state entries in the recorder database due to cast updates every 11 seconds or so.

Change it so that some state entries are skipped when position hasn't changed.

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If the code does not interact with devices:

Tests have been added to verify that the new code works.

balloob · 2018-04-09T03:27:40Z

I am confused. I was already under the impression that we did not update the state of an entity if the position remains the same / can be calculated because player state hasn't changed 🤔

CC @OttoWinter

dersger · 2018-04-09T06:04:10Z

The component was recently changed to be polling. I believe it's a result of that.

But your comment made me think. Maybe we instead should have some generic handling of this in MediaPlayerDevice.state_attributes()?

balloob · 2018-04-09T15:08:44Z

It's difficult to make this generic as every media player has a different way of getting data. Cast is push, VLC is polling etc.

dersger · 2018-04-09T15:52:19Z

But inside MediaPlayerDevice.state_attributes() it doesn't matter if it's push or poll. What I mean is that the code there could get the old state from self.hass.states, compare it to the new state_attr from the component, and then modify the return value to keep the old position and update time in cases where an update would be unnecessary. It would then fix this type of problem for all media players.

But maybe it's a bit unconventional to change the state attributes inside MediaPlayerDevice.state_attributes().

OttoWinter · 2018-04-09T19:44:27Z

The cast integration is now "semi-polling" indeed (since #13275, tl;dr in theory, the cast protocol notifies us of all media state changes, but in practice quite a few apps including netflix never send those callbacks out 😕). We're only in polling mode iff an app is active, but yes the issue is definitely caused by this.

So to summarise, the reason this is happening in the first place is because of media_status_position. Normally, states are never updated in the state machine if they're identical to the previous state. This is in order to keep the state machine from being flooded by polling integrations. But the state machine state equal checking code of course doesn't know about media_status_position. If we schedule a new state change it will of course have a new position and therefore also a state that is different from the last one.

I'm not 100% sure how this should be handled. Doing this inside the cast integration just seems quite messy and potentially lots of other polling media_player implementations will just copy the code over. On the other hand, I don't think there's currently a good way component.media_player.__init__ can override the state equality behavior... Another option would potentially be to store the media_started_at = now - media_position, though that would be breaking and definitely would make machine learning a bit more difficult.

dersger · 2018-04-09T20:03:40Z

I've been thinking about how to change MediaPlayerDevice.state_attributes() to handle all media players. This is totally untested but can at least show my idea:

    def state_attributes(self):
        """Return the state attributes."""
        if self.state == STATE_OFF:
            return None

        state_attr = {
            attr: getattr(self, attr) for attr
            in ATTR_TO_PROPERTY if getattr(self, attr) is not None
        }

        old_state = self.hass.states.get(self.entity_id)
        if old_state and (self.state == old_state.state and
                          ATTR_MEDIA_POSITION_UPDATED_AT in state_attr and
                          ATTR_MEDIA_POSITION in state_attr and
                          ATTR_MEDIA_POSITION_UPDATED_AT in old_state.attributes and
                          ATTR_MEDIA_POSITION in old_state.attributes):
            if self.state == STATE_PLAYING:
                pos_diff = state_attr[ATTR_MEDIA_POSITION] - old_state.attributes[ATTR_MEDIA_POSITION]
                time_diff = (state_attr[ATTR_MEDIA_POSITION_UPDATED_AT] -
                             old_state.attributes[ATTR_MEDIA_POSITION_UPDATED_AT]).total_seconds()
                if abs(pos_diff - time_diff) < 1.5:
                    state_attr[ATTR_MEDIA_POSITION] = old_state.attributes[ATTR_MEDIA_POSITION]
                    state_attr[ATTR_MEDIA_POSITION_UPDATED_AT] = old_state.attributes[ATTR_MEDIA_POSITION_UPDATED_AT]
            elif state_attr[ATTR_MEDIA_POSITION] == old_state.attributes[ATTR_MEDIA_POSITION]:
                state_attr[ATTR_MEDIA_POSITION_UPDATED_AT] = old_state.attributes[ATTR_MEDIA_POSITION_UPDATED_AT]

        return state_attr

I think both the advantage and disadvantage of that is that the component will be in less control of the attribute updates.

houndci-bot · 2018-04-10T18:20:22Z

+z"""The tests for the Cast Media player platform."""
 # pylint: disable=protected-access
 import asyncio
+import datetime as dt


module level import not at top of file

houndci-bot · 2018-04-10T18:20:22Z

@@ -1,6 +1,7 @@
-"""The tests for the Cast Media player platform."""
+z"""The tests for the Cast Media player platform."""


SyntaxError: invalid syntax

houndci-bot · 2018-04-10T18:34:47Z

+        entity.new_media_status(media_status)
+        await hass.async_block_till_done()
+    assert ATTR_MEDIA_POSITION not in state.attributes
+    assert ATTR_MEDIA_POSITION_UPDATED_AT not in state.attributes


undefined name 'ATTR_MEDIA_POSITION_UPDATED_AT'

houndci-bot · 2018-04-10T18:34:47Z

+    with patch('homeassistant.util.dt.utcnow', return_value=now_plus_20):
+        entity.new_media_status(media_status)
+        await hass.async_block_till_done()
+    assert ATTR_MEDIA_POSITION not in state.attributes
