Skip to content

Add use_wallclock_as_timestamps option to generic#71245

Merged
balloob merged 12 commits intohome-assistant:devfrom
uvjustin:use-wallclock-as-timestamps-generic
May 12, 2022
Merged

Add use_wallclock_as_timestamps option to generic#71245
balloob merged 12 commits intohome-assistant:devfrom
uvjustin:use-wallclock-as-timestamps-generic

Conversation

@uvjustin
Copy link
Copy Markdown
Contributor

@uvjustin uvjustin commented May 3, 2022

Proposed change

This PR allows generic to rewrite camera timestamps using the use_wallclock_as_timestamps ffmpeg option. Use of this option helps correct various issues including sporadic crashes due to packet corruption and HLS segmenting issues with audio+video streams.

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

Additional information

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:

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.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link
Copy Markdown

Hey there @davet2001, mind taking a look at this pull request as it has been labeled with an integration (generic) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@davet2001
Copy link
Copy Markdown
Contributor

@uvjustin This looks like a good addition.

I have been wondering whether many of these options could be listed only in the options flow. The reason is that the first screen is already huge. Perhaps this and the frame rate could be set to a default value, then the user can change them via 'configuration' if they want/need to.

@uvjustin
Copy link
Copy Markdown
Contributor Author

uvjustin commented May 4, 2022

@uvjustin This looks like a good addition.

I have been wondering whether many of these options could be listed only in the options flow. The reason is that the first screen is already huge. Perhaps this and the frame rate could be set to a default value, then the user can change them via 'configuration' if they want/need to.

Sounds good. I'll work on #71247 first. I think moving the frame rate setting to the options flow should probably just be a separate PR.

@davet2001
Copy link
Copy Markdown
Contributor

Yes, agreed.

@uvjustin uvjustin force-pushed the use-wallclock-as-timestamps-generic branch from 2abb4d4 to 127bd68 Compare May 9, 2022 12:06
@uvjustin uvjustin marked this pull request as ready for review May 9, 2022 12:29
@uvjustin uvjustin requested a review from davet2001 as a code owner May 9, 2022 12:29
@uvjustin
Copy link
Copy Markdown
Contributor Author

uvjustin commented May 9, 2022

@davet2001 Maybe we should do this one before #71247 so it will be easier to cherry pick for a patch release

Copy link
Copy Markdown
Contributor

@davet2001 davet2001 left a comment

Choose a reason for hiding this comment

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

Looks pretty good - a couple of remaining minor comments.

@uvjustin uvjustin force-pushed the use-wallclock-as-timestamps-generic branch from 4f545cf to bd3e214 Compare May 10, 2022 03:55
Copy link
Copy Markdown
Contributor

@davet2001 davet2001 left a comment

Choose a reason for hiding this comment

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

Looking good!

Just minor things remaining.

@uvjustin uvjustin added this to the 2022.5.4 milestone May 11, 2022
@balloob
Copy link
Copy Markdown
Member

balloob commented May 12, 2022

I see this got tagged for the milestone but isn't this a new feature to work around existing issues?

Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
@uvjustin
Copy link
Copy Markdown
Contributor Author

I see this got tagged for the milestone but isn't this a new feature to work around existing issues?

Yes, I thought that since it fixed an issue it could be released as a milestone. But I assume from your comment that since it's a new feature, that it has to wait until the next version?
It's a pretty serious issue for some users, as it can cause HA to crash quite often. It hasn't affected me since I have been using the option locally since January, but my PR then was rejected as changes to yaml were forbidden. Ever since then generic has been a moving target as @davet2001 has been augmenting the config flow.

@uvjustin
Copy link
Copy Markdown
Contributor Author

@hunterjm @allenporter
BTW for more context on why it can cause crashes: ffmpeg assertions can cause a C abort, and I don't think there's a way to catch that in python. Trying to mimic the assertion preemptively in python is not feasible as it would require replicating quite a lot of the timestamp calculations.
Luckily this seems to be the main issue where an assertion failure crash shows up. If other issues arise, we might have to try to patch the assertion upstream or consider changing the stream model from a threading model to a process model.

@balloob balloob merged commit 7e49ae6 into home-assistant:dev May 12, 2022
balloob added a commit that referenced this pull request May 12, 2022
Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
Co-authored-by: Paulus Schoutsen <balloob@gmail.com>
@balloob balloob mentioned this pull request May 13, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 14, 2022
@frenck
Copy link
Copy Markdown
Member

frenck commented May 16, 2022

This is a new feature and should never ended up in a patch release. Even if a new feature "fixes" an issue.

@uvjustin
Copy link
Copy Markdown
Contributor Author

Noted, sorry about that.

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.

Home Assistant is repeatedly restarting after a critical error in the Stream component

5 participants