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

Implement video filtering with libavfilter #68

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KingOfTheBlues
Copy link

Adds bindings for libavfilter attempting to follow established patterns where possible. Audio filters to be added in later.

The implementation is based on the ffmpeg example: https://www.ffmpeg.org/doxygen/trunk/filtering_8c-source.html

Thanks in advance for your time considering this PR and providing any feedback.

@KingOfTheBlues KingOfTheBlues force-pushed the feat/implement-video-filtergraph branch from 8560306 to f2f8242 Compare April 3, 2023 08:14
@operutka
Copy link
Member

Hi @KingOfTheBlues and thank you for the PR. I've checked it just briefly and it's looking good. I'll have to find some time to look into it in detail though.

Until then, please put avfilter behind a feature gate. Without it, we could be breaking existing builds by introducing a new link time dependency which may not be available (especially if a custom build of FFmpeg is being used).

@operutka operutka self-requested a review April 16, 2023 10:14
@operutka operutka self-assigned this Apr 16, 2023
@operutka operutka added the enhancement New feature or request label Apr 16, 2023
@KingOfTheBlues KingOfTheBlues force-pushed the feat/implement-video-filtergraph branch 3 times, most recently from f2f8242 to a89529c Compare April 18, 2023 15:01
ac-ffmpeg/src/codec/video/filter.c Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.rs Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.rs Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.rs Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.rs Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.c Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.c Outdated Show resolved Hide resolved
ac-ffmpeg/src/codec/video/filter.c Outdated Show resolved Hide resolved
Comment on lines +109 to +118
int ret = av_buffersrc_add_frame(context, frame);

if (ret == 0 || ret == AVERROR_EOF) {
return 1;
}
else if (ret == AVERROR(EAGAIN)) {
return 0;
}

return ret;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is correct. The FFmpeg documentation does not say that av_buffersrc_add_frame would behave the same way as avcodec_send_frame. Is it possible that pushing one frame to a filter input can generate multiple frames at the filter output? Do we also have to consume all output frames before pushing the next input frame (i.e. is it possible that av_buffersrc_add_frame would return AVERROR(EAGAIN))?

Copy link
Author

Choose a reason for hiding this comment

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

thought I had addressed this one, actually I reverted the change for now as applying the respective change in the C code yields "all frames must be consumed before flushing" from CodecError

ac-ffmpeg/src/codec/video/filter.rs Outdated Show resolved Hide resolved
@KingOfTheBlues
Copy link
Author

thanks so much @operutka for the awesome feedback! The code is now greatly simplified and I really appreciate the help here. I believe I have addressed all your comments now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants