Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PyAV fork and set hvc1 codec tag for H.265 #58309

Merged
merged 4 commits into from Oct 27, 2021

Conversation

uvjustin
Copy link
Contributor

Proposed change

Using the hvc1 codec tag to mux H.265 video gets it working on Edge and browsers which support H.265.
I merged a PR which adds the codec tag functionality to the PyAV repo back in February, but there has not been a new release of the library since then.
The owner of the repo has indicated that the two maintainers are both very busy and the project is primarily for their own use, so it's not clear when a new release will be published.
I have forked the repo and published it under ha-av. The version that I used was created directly from this CI run https://github.com/uvjustin/PyAV/runs/3976952679 with minimal changes from the original fork. As of now I do not intend to maintain a fork and we can switch back to the official PyAV library if and when they release a new version.
This PR switches the stream dependency from av to ha-av and uses the hvc1 codec tag when H.265 video is detected. This should get H.265 video playing in Edge on devices with hardware support.

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

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)

@project-bot project-bot bot moved this from Needs review to By Code Owner in Dev Oct 23, 2021
@allenporter
Copy link
Contributor

Changelog of old pyav to new vesion:
uvjustin/PyAV@v8.0.3...v8.0.4-rc.1

@allenporter
Copy link
Contributor

Given the importance of stream to Home Assistance, I think it makes sense for others review this also.

The discussion around maintenance status was here: PyAV-Org/PyAV#819

It sounds like they would be interested in having someone else maintain the project, but my impression is that you aren't necessarily looking to do that. However, given you were willing to do a fork and get a release out, maybe they'd let you do the same for PyAV without taking over full ownership? :)

The ha-av pypi project https://pypi.org/project/ha-av/ still has pointers to the original project, though I guess you're saying since this is temporary.

My intuition is to commit one way or the other: to either maintain ha-av or help get a pyav release out.

@allenporter allenporter added the second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. label Oct 25, 2021
@project-bot project-bot bot moved this from By Code Owner to Second opinion wanted in Dev Oct 25, 2021
@uvjustin
Copy link
Contributor Author

It sounds like they would be interested in having someone else maintain the project, but my impression is that you aren't necessarily looking to do that. However, given you were willing to do a fork and get a release out, maybe they'd let you do the same for PyAV without taking over full ownership? :)

I actually asked on gitter about the possibility of a new release, but I didn't get a response after about a month so I just deleted the message and created this fork. The maintainers are busy and don't have a need to update things over there, and that is their prerogative.
In the discussion you linked, it seems like the path towards becoming a maintainer is not clear cut and might take quite some time.

The ha-av pypi project https://pypi.org/project/ha-av/ still has pointers to the original project, though I guess you're saying since this is temporary.

Yes. I can update those if necessary but want to avoid too many commits. Right now it's clear the only deviation from the original repo is the pypi name change and the changes to get CI working.

My intuition is to commit one way or the other: to either maintain ha-av or help get a pyav release out.

I see your point but I would like to get this commit merged sooner rather than later. It's been in the works since February when I made the corresponding PR on the PyAV repo. Can we consider this PR without committing to a longer term decision on the library? The next HA beta is cut on Wednesday, and if we miss that we won't get this feature until the Dec release at the earliest.

@allenporter
Copy link
Contributor

OK, so sounds like we have an answer about a pyav release. Going with a fork makes sense.

The part i'm having trouble reconciling though is that you don't intend to maintain the fork, however by using this fork we're effectively committing to this approach IMO. I don't think we need to figure out a long term plan, just that we need to confirm we're OK with this decision being the long term plan. I think its safer to assume this will be the status quo for a long time.

@uvjustin
Copy link
Contributor Author

To clarify: I will keep the fork open, but I'm not interested in diverging from the PyAV repo too much. What I want to avoid is adding more features and also getting non HA devs using our fork instead of the original repo. The point is not to replace or compete with the original repo.
I think the library as is is sufficient for our uses for the foreseeable future, so there's not a need to add much. The only features we might add are the typing fixes from @cdce8p , bitstream filters, and hardware acceleration (the first has an outstanding PR since April, and the second and third have branches that were started but not merged to main), but those are wants more than needs. By not diverging much from the original repo we can easily switch back when the original library is updated.
@hunterjm Any thoughts on this?

@allenporter
Copy link
Contributor

Thanks for the detailed explanation, that seems like a fine state to me. I'm supportive, and would love to hear from others.

@frenck frenck added the smash Indicator this PR is close to finish for merging or closing label Oct 25, 2021
@hunterjm
Copy link
Member

I agree that we shouldn't diverge from pyAV too much. It would be nice if they made you a maintainer :)

Otherwise I'm ok with this as well.

@uvjustin
Copy link
Contributor Author

Are we ok to approve/merge this?

Copy link
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.

Yes, thanks for driving forward progress here 馃憤馃徏

Dev automation moved this from Second opinion wanted to Reviewer approved Oct 27, 2021
@allenporter
Copy link
Contributor

It sounds like what would be required to become a maintainer is to effectively start reviewing all the PRs.

Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

Considering the upstream maintainer discussion, I think it is fine to fork for now. The fork will also show the capabilities the maintainers are looking for (make sure to contribute them back in PRs to the original repo to make it visible).

@frenck frenck merged commit 35acca1 into home-assistant:dev Oct 27, 2021
Dev automation moved this from Reviewer approved to Done Oct 27, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Oct 28, 2021
@uvjustin uvjustin deleted the use-ha-av branch November 7, 2021 11:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
by-code-owner cla-signed core Hacktoberfest has-tests integration: stream new-feature second-opinion-wanted Add this label when a reviewer needs a second opinion from another member. small-pr PRs with less than 30 lines. smash Indicator this PR is close to finish for merging or closing
Projects
Dev
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants