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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audio buffer support in filter graph #389

Closed
wants to merge 30 commits into from

Conversation

egao1980
Copy link
Contributor

No description provided.

@mikeboers
Copy link
Member

This is looking quite lovely! Thank you.

This line lies, but that is easy to change or add that feature like the VideoFrame variant.

My only lingering feeling is: since the planar audio comes back as 2D, should the non-planar audio do the same (except the axis be reversed)?

av/filter/graph.pyx Outdated Show resolved Hide resolved
av/audio/frame.pyx Outdated Show resolved Hide resolved
av/audio/frame.pyx Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
av/filter/graph.pyx Outdated Show resolved Hide resolved
@egao1980
Copy link
Contributor Author

I've added a ported filter_audio example. The filter chain looks usable now. Please review.

@jlaine
Copy link
Member

jlaine commented Oct 29, 2018

I'm not familiar with the libav terminology but what is "abuffer"? Unless this is the exact name of something in libav, "audio buffer" would be more explicit

@mikeboers
Copy link
Member

@jlaine Unfortunately, it really is called "abuffer".

@egao1980
Copy link
Contributor Author

It looks like appveyor 3.5 build is failing on random dependency download error. Is it possible to trigger rebuild?

@egao1980
Copy link
Contributor Author

egao1980 commented Dec 5, 2018

Could someone please re-review this one so it could be merged.

@jlaine
Copy link
Member

jlaine commented Dec 5, 2018

I think this is very cool, but as there are zero tests for this new code, I'm not comfortable with merging it. Doubly so as there are a lot of conditional branches..

av/filter/graph.pyx Show resolved Hide resolved
av/filter/graph.pyx Show resolved Hide resolved
av/filter/graph.pyx Outdated Show resolved Hide resolved
av/filter/graph.pyx Outdated Show resolved Hide resolved
examples/filter_audio.py Outdated Show resolved Hide resolved
@egao1980
Copy link
Contributor Author

egao1980 commented Dec 6, 2018

Hi Jeremy, the Graph part is currently undertested anyway so I'll just add a few tests for audio filters.

@jlaine
Copy link
Member

jlaine commented Dec 6, 2018

Right the current filter graph code is untested, otherwise the "elif" error would have been caught. We are currently trying to raise the test coverage so any new code must come with tests.

@morrolinux
Copy link

Any updates on this?
Since I really need this feature I'm currently using this branch from @egao1980 's Fork. working flawlessly for now

@egao1980
Copy link
Contributor Author

Hi @morrolinux, I'm currently lack free time to write proper set of unit tests for this one. I'll fix the conflicts though.

@mikeboers
Copy link
Member

Thanks @egao1980 for trying to do that. My apologies for rarely having free time either; paying the bills has to come first, unfortunately.

@tetelestia
Copy link

Just bumping this up, hoping it could be merged in. I've been using this branch for a while with no problems.

@egao1980
Copy link
Contributor Author

TUW-GEO/pytesmo#177

@egao1980
Copy link
Contributor Author

Please re-review as I've added error codes, fixed example, and most importantly - added a few unit tests covering audio filters.

Please note that several Travis configurations fail to initialise even before actually building the project.

@mikeboers
Copy link
Member

I'm interested in working through this, but it really turned into a monolith.

Anything that isn't related to the specific purpose of this has to be dealt with elsewhere (e.g. build and test fixes). The error codes stuff sits particularly badly with me, as it stretches so much further than audio buffers and was complete unnecessary AFAICT.

If someone wants to champion this, it will need to be based off of the current develop (not with the current develop merged into it!), and be isolated to the audio buffer support.

I'll try to take a look at it myself, as I'm on a bit of a PyAV binge, but I make no promises. I honestly don't know the scope of it because the history as-is is way too convoluted. If I do it, I might start from scratch and lose the authorship information.

@mikeboers mikeboers mentioned this pull request Oct 18, 2019
@mikeboers
Copy link
Member

Closing in favour of #562.

@mikeboers mikeboers closed this Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants