Skip to content

Decouple stream options from PyAV options#71247

Merged
balloob merged 12 commits intohome-assistant:devfrom
uvjustin:sanitize-stream-options
May 15, 2022
Merged

Decouple stream options from PyAV options#71247
balloob merged 12 commits intohome-assistant:devfrom
uvjustin:sanitize-stream-options

Conversation

@uvjustin
Copy link
Copy Markdown
Contributor

@uvjustin uvjustin commented May 3, 2022

Breaking change

create_stream() takes an options dict and passes it straight through to PyAV, which passes it straight into the ffmpeg libraries. This PR adds an intermediate layer through the new STREAM_OPTIONS_SCHEMA and convert_stream_options() function to decouple the options used internally in stream from the options used in ffmpeg. This allows us to 1) restrict the options used to a subset we have tested while also sanitizing them if necessary, 2) change the syntax of the stream options, and potentially 3) group some PyAV options together in a single stream option.
This PR doesn't break anything in core, as the appropriate changes have been made to generic and ONVIF, but this may break some custom components which implement Camera and use the stream_options member to pass in custom PyAV options.

Proposed change

Currently create_stream() takes an options dict which (after having two options added) passes through to the Stream constructor, which in turn passes the options through the stream worker, eventually ending up in the PyAV av.open() function. These options don't seem to be used in the HA code base are used in ONVIF and generic, so those uses were changed here, but it's possible that some custom components also use them.
This PR removes passing the options through directly, decoupling the camera/stream interface from the PyAV API. (Note that currently only one option is supported, and that option is still effectively passed through as is.) If desired, specific options can be defined and checked against in create_stream and then added to the options dict. For example, #71245 can add a boolean option {"rewrite_timestamps" : True} instead of having to know about the PyAV option {"use_wallclock_as_timestamps": "1"}

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

  • This PR fixes or closes issue: fixes #
  • 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:

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 @hunterjm, @allenporter, mind taking a look at this pull request as it has been labeled with an integration (stream) you are listed as a code owner for? Thanks!
(message by CodeOwnersMention)

@uvjustin uvjustin marked this pull request as draft May 3, 2022 15:00
@uvjustin uvjustin force-pushed the sanitize-stream-options branch from e548e2e to d83bda6 Compare May 4, 2022 11:55
"rtsp_flags": "prefer_tcp",
"stimeout": "5000000",
**options,
"timeout": "5000000",
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

stimeout was deprecated/renamed as of ffmpeg 4.0 and will be removed in ffmpeg 5.0:
FFmpeg/FFmpeg@ff46124
FFmpeg/FFmpeg@6f34f03

@uvjustin uvjustin changed the title Do not pass options directly from create_stream to Stream constructor Decouple stream options from PyAV options May 4, 2022
@uvjustin uvjustin marked this pull request as ready for review May 5, 2022 00:44
@uvjustin uvjustin requested a review from davet2001 as a code owner May 5, 2022 00:44
@uvjustin uvjustin marked this pull request as draft May 5, 2022 03:45
@uvjustin uvjustin marked this pull request as ready for review May 5, 2022 05:27
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.

Overall looks pretty good to me, just some minor questions.

Copy link
Copy Markdown
Contributor

@allenporter allenporter left a comment

Choose a reason for hiding this comment

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

Looks great, appreciate this as it will definitely make it more clear how this works for new folks reading the code. 👍🏼

I have a couple minor comments.

@allenporter
Copy link
Copy Markdown
Contributor

I believe we have a new best practice for changes like this which would be:

(I don't think there is widespread use of ffmpeg flags or use of stream in custom components, but just in case it can help.)

@uvjustin uvjustin marked this pull request as draft May 10, 2022 08:14
@uvjustin
Copy link
Copy Markdown
Contributor Author

Marking as draft pending #71245

@uvjustin uvjustin force-pushed the sanitize-stream-options branch from 48e031b to 1b4d648 Compare May 13, 2022 02:34
@uvjustin uvjustin marked this pull request as ready for review May 13, 2022 02:35
@balloob balloob merged commit 617b0d0 into home-assistant:dev May 15, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2022
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.

7 participants