architecture icon indicating copy to clipboard operation
architecture copied to clipboard

Simplify media player media position attribute

Open emlove opened this issue 7 years ago • 10 comments

Background

Currently for media player platforms that wish to expose the media player position, we ask them to implement two properties, media_position, and media_position_updated_at. Since we know that media_position normally is constantly updating, the goal was to reduce unnecessary state changes on every poll. If media is playing normally, platforms could report the same values for media_position and media_position_updated_at to indicate that the media has continued to play and an unnecessary state change can be avoided.

The problem

The problem here is that most APIs to read media player status don't provide this type of information by default, and only provide a media position. We get a lot of PRs that implement this just by returning the media position untouched, and returning now for media_position_updated_at on every state change, which works but defeats the original purpose for this API. This is sometimes caught in review, but results in us asking every platform to perform the same workarounds to convert the device API to this format.

The proposal

I propose that for the media player base entity class, we simplify the API that platforms need to implement to just return the media position unmodified from the external device status. Then, when the media player base class is updating its state, we can implement the necessary data adjustments on the core media player entity class. When we first get a media_position on an update, we will record now as the updated time and save that into the state machine matching the current format. On the next state update, we will read the current media_position reported from the platform. If the previous media_position plus the difference between now and the previous media_position_updated_at is the same as the new media_position reported by the platform, we'll rewrite the original values back to the state machine unchanged. This will allow the core component to manage the specifics of avoiding unnecessary state changes, and platforms can simply pass through the media position from their devices.

emlove avatar Nov 07 '18 00:11 emlove

I like this. Note that there are some platforms that actually support our format, Cast comes specifically to mind.

balloob avatar Nov 07 '18 10:11 balloob

If the previous media_position plus the difference between now and the previous media_position_updated_at is the same as the new media_position reported by the platform, we'll rewrite the original values back to the state machine unchanged.

Sounds good, but won't it happen a lot that due to latency/network delays/high load, this value won't exactly match the previous value + the difference? This will still cause a new entry in the state machine.

michaelarnauts avatar Nov 07 '18 15:11 michaelarnauts

Yeah that's possible, and I'd be open to a slightly fuzzy equality check myself, maybe we keep it if it's within 1 or 2 seconds of the previous value to account for small jitter.

emlove avatar Nov 07 '18 15:11 emlove

1-2 sec is too much tolerance, as I could have paused and resumed in that window and it’s be out of sync.

Is this complexity really needed? Meaning, is there a known/observable issue with updating the state machine at every position update? (Which I assume is no more often than once per second).

andrewsayre avatar Mar 21 '19 00:03 andrewsayre

is there a known/observable issue with updating the state machine at every position update?

For one thing, it will cause the recorder to use significantly more database space and slow down history queries.

jjlawren avatar Mar 21 '19 02:03 jjlawren

@andrewsayre it will cause a lot of unnecessary traffic and also be stored in recorder.

balloob avatar Mar 21 '19 17:03 balloob

Gotcha - didn't think about the recorder. I think simplifying the API/interface for platforms is going to be important and probably need to offer both options if platforms do work like Cast.

This is going to prove somewhat challenging to solve correctly for HEOS based on how it updates, but we'll figure it out!

andrewsayre avatar Mar 21 '19 17:03 andrewsayre

Hi, I just had an SD card totally consumed after some months on a RPI. Trying to digging into the cause, I noticed that the home-assistant_v2.db was a bit too big (1GB with 7 days retention) for the HA load. Looking up into the events table, I got a huge amount of "state_changed" events on my chromecast while playing content, one each 0.2 - 0.3 seconds, where the only changed attributes are "media_position" and "media_position_updated_at". To avoid to wear to death the new SD card, I had to disable the media_player domain from the recorder component, but this is somewhat not ideal. So please, keep up with this improvement. Maybe it could worth a try to implement a filter in the recorder component?

lukakama avatar Mar 14 '20 14:03 lukakama

The excessive position updates should really be filtered at the source in the media player component.

Edit: I forgot what this proposal was about, my comment was redundant in that context.

jjlawren avatar Mar 14 '20 14:03 jjlawren

It looks to be related to this bug, but there are plenty of circumstances with the new youtube app builtin to chromecast, where when a playlist stops due to eg action within the user's youtube app on their phone, state doesn't go to "paused" or "idle", but "media_position" and "media_position_updated_at" don't increment.

I think a way to trigger this condition is simply to pause a video in the phone app (state goes to paused paused), press back, press close in the little popup at the bottom, press "close player" where state goes back to "playing", chromecast goes back to a vanilla youtube screen, but media_player thinks the state is this:

{
  "entity_id": "media_player.loungeroom_tv",
  "state": "playing",
  "attributes": {
    "volume_level": 0.800000011920929,
    "is_volume_muted": false,
    "media_content_id": "udM9VlvATQw",
    "media_duration": 964.961,
    "media_position": 6.156,
    "media_position_updated_at": "2021-12-26T09:05:12.892607+00:00",
    "media_title": "Villagers React To Scale Of Universe ! Tribal People React To Universe Is Way Big
ger Than You Think",
    "app_id": "233637DE",
    "app_name": "YouTube",
    "entity_picture_local": "/api/media_player_proxy/media_player.loungeroom_tv?token=yyyy&cache=zzzz",
    "entity_picture": "https://i.ytimg.com/vi/udM9VlvATQw/maxresdefault.jpg",
    "friendly_name": "Loungeroom TV",
    "supported_features": 152511
  },
  "last_changed": "2021-12-26T09:05:12.894085+00:00",
  "last_updated": "2021-12-26T09:05:12.894085+00:00",
  "context": {
    "id": "xxxx",
    "parent_id": null,
    "user_id": null
  }
}

spacelama avatar Dec 26 '21 09:12 spacelama

This architecture issue is old, stale, and possibly obsolete. Things changed a lot over the years. Additionally, we have been moving to discussions for these architectural discussions.

For that reason, I'm going to close this issue.

../Frenck

frenck avatar May 11 '23 14:05 frenck