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

Add bit stream filter support #61

Merged
merged 8 commits into from
May 14, 2024
Merged

Conversation

leandromoreira
Copy link
Contributor

Add support to Bit Stream Filter.

Bitstream filters transform encoded media data without decoding it. This allows e.g. manipulating various header values. Bitstream filters operate on AVPackets.

The bitstream filtering API is centered around two structures: AVBitStreamFilter and AVBSFContext. The former represents a bitstream filter in abstract, the latter a specific filtering process. Obtain an AVBitStreamFilter using av_bsf_get_by_name() or av_bsf_iterate(), then pass it to av_bsf_alloc() to create an AVBSFContext. Fill in the user-settable AVBSFContext fields, as described in its documentation, then call av_bsf_init() to prepare the filter context for use.

Submit packets for filtering using av_bsf_send_packet(), obtain filtered results with av_bsf_receive_packet(). When no more input packets will be sent, submit a NULL AVPacket to signal the end of the stream to the filter. av_bsf_receive_packet() will then return trailing packets, if any are produced by the filter.

Finally, free the filter context with av_bsf_free().

src: https://github.com/FFmpeg/FFmpeg/blob/release/5.1/libavcodec/bsf.h

Copy link
Owner

@asticode asticode left a comment

Choose a reason for hiding this comment

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

Thanks for the PR ❤️

I've made some comments 👍

Also, could you add tests?

  • for the bit stream filter, the best would be to add a Name() string function (and a String() string function that would return the result of .Name()) in the struct, and in the test find a filter available by default and assert its name.
  • for the bit stream filter context, the best would be to test allocating a context, initializing it and freeing it. Before freeing it, that would be a good idea to test the getters and setters as well (for the Class you can use something similar to this). If you find a way to test Send/ReceivePacket that would be even better, but that's optionnal.

bit_stream_filter.go Outdated Show resolved Hide resolved
bit_stream_filter_context.go Outdated Show resolved Hide resolved
bit_stream_filter_context.go Outdated Show resolved Hide resolved
bit_stream_filter_context.go Outdated Show resolved Hide resolved
bit_stream_filter_context.go Outdated Show resolved Hide resolved
bit_stream_filter_context.go Outdated Show resolved Hide resolved
Copy link
Contributor Author

@leandromoreira leandromoreira left a comment

Choose a reason for hiding this comment

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

done

@asticode
Copy link
Owner

Nice! Did you see the part of my comment mentioning tests as well?

@leandromoreira
Copy link
Contributor Author

leandromoreira commented May 13, 2024

Thanks for the PR ❤️

I've made some comments 👍

Also, could you add tests?

* for the bit stream filter, the best would be to add a `Name() string` function (and a `String() string` function that would return the result of .Name()) in the struct, and in the test find a filter available by default and assert its name.

* for the bit stream filter context, the best would be to test allocating a context, initializing it and freeing it. Before freeing it, that would be a good idea to test the getters and setters as well (for the Class you can use something similar to [this](https://github.com/asticode/go-astiav/blob/master/format_context_test.go#L30)). If you find a way to test Send/ReceivePacket that would be even better, but that's optionnal.

Will add tests later :) please don't merge it yet.

@leandromoreira
Copy link
Contributor Author

The tests I introduced are passing:

 === RUN   TestBitStreamFilterContext
--- PASS: TestBitStreamFilterContext (0.00s)
=== RUN   TestBitStreamFilter
--- PASS: TestBitStreamFilter (0.00s)

They shouldn't cause any side effect though.

bit_stream_filter_test.go Outdated Show resolved Hide resolved
bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
astiav_test.go Outdated Show resolved Hide resolved
bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
@asticode
Copy link
Owner

The tests I introduced are passing:

 === RUN   TestBitStreamFilterContext
--- PASS: TestBitStreamFilterContext (0.00s)
=== RUN   TestBitStreamFilter
--- PASS: TestBitStreamFilter (0.00s)

They shouldn't cause any side effect though.

Yeah testing SendPacket and ReceivePacket like this doesn't feel right, but I have no alternative for now. Could add // TODO Test SendPacket and ReceivePacket at the end of TestBitStreamFilterContext in bit_stream_filter_context_test.go though?

@asticode
Copy link
Owner

Windows tests seem to be failing (without any error message, that would be too easy otherwise 🤦 ), I'll take a look

@leandromoreira
Copy link
Contributor Author

Windows tests seem to be failing (without any error message, that would be too easy otherwise 🤦 ), I'll take a look

:( I don't have a windows to test, otherwise I'd try to help.

BTW, is it easy to run tests locally on macos?

@asticode
Copy link
Owner

:( I don't have a windows to test, otherwise I'd try to help.

I'm using Github actions to run tests on windows and linux

BTW, is it easy to run tests locally on macos?

Sure, as long as ffmpeg is installed and the proper environment variables are setup, a simple go test does the trick

bit_stream_filter_context_test.go Outdated Show resolved Hide resolved
@asticode asticode merged commit d884495 into asticode:master May 14, 2024
3 checks passed
@asticode
Copy link
Owner

Thanks again for the PR! ❤️

Let me know whether you need a tag 👍

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

Successfully merging this pull request may close these issues.

None yet

2 participants