Skip to content

Conversation

@hunterjm
Copy link
Member

@hunterjm hunterjm commented Apr 24, 2020

Breaking change

pyAV >7.0.0 drops support for FFmpeg versions <4.0. FFmpeg will need to be updated to at least 4.0 for stream to work. If you run supervised or docker, no manual intervention is needed. Alternative installs can check the stream integration documentation for troubleshooting tips.

Proposed change

Bump the pyAV lib to 7.0.1 which exposes new flags that can be used to fix the recording duration as stored in the file header. This was tested locally and fixed the issue layed out in #34605. The existing tests that are marked as skipped were included when running pytest locally and all passed.
Screenshot from 2020-04-24 11-19-06

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
stream:

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.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

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

@hunterjm
Copy link
Member Author

@pvizeli - It looks like we may need to update the version of FFMPEG on our CI images. The official Home Assistant docker images using Alpine should meet the FFMPEG requirement for this library without issue.

@pvizeli pvizeli added this to the 0.109.0 milestone Apr 24, 2020
@balloob balloob removed this from the 0.109.0 milestone Apr 24, 2020
@hunterjm
Copy link
Member Author

This doesn't need to go out in 0.109 since beta is already cut. It's a low impact bugfix.

@balloob
Copy link
Member

balloob commented Apr 24, 2020

Untagged this from the milestone. It only fixes a rare case issue. Probably better wait until next release? (unless you've already updated all images Pascal, in that case proceed)

@pvizeli
Copy link
Member

pvizeli commented Apr 24, 2020

@balloob we support ffmpeg4 a long time. Just the CI not. I updated it, so the CI should work now. But I'm fine with that

@hunterjm
Copy link
Member Author

hunterjm commented Apr 24, 2020

Not sure what to do with code coverage here. Tests are written that cover this code, and they work as intended when enabled locally. They were disabled back when we were on CircleCI because they were deemed flaky.

Edit: If we want to keep them disabled, I can add them to .coveragerc

@balloob balloob merged commit 40d3d64 into home-assistant:dev Apr 24, 2020
@lock lock bot locked and limited conversation to collaborators Apr 25, 2020
@hunterjm hunterjm deleted the stream-recording-header branch April 28, 2020 19:24
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.

CAMERA.RECORD from STREAM - Invalid Video Length property

4 participants