Add codec ffmpeg #621

3DRX · 2025-04-08T18:23:00Z

This is a PR for adding ffmpeg based video encoders.

Documentation, since ffmpeg library is introduced as dependency, building requires CGO so it's not a working-out-of-the-box experience.
~~Update CI pipeline, or it will failed for not founding ffmpeg library.~~ We add go-astiav as a dependency in pkg/codec/ffmpeg/go.mod to avoid breaking CI and other user's build. When using github.com/pion/mediadevices/pkg/codec/ffmpeg, it's a requirement for users to provide their own ffmpeg library as described here.
Add test. For now, changing image size on the fly is not supported, so TestImageSizeChange is skipped. Also, since CI environment can't have access to hardware encoders such as NVENC, only software encoder is tested.
Update CI to run new test.
- Runs tests for pkg/codec/ffmpeg in CI.
- Cache FFmpeg build.
- Configure codecov result and other things in CI.

codecov · 2025-04-10T13:54:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 44.49%. Comparing base (c4fd28c) to head (d8741c0).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #621   +/-   ##
=======================================
  Coverage   44.49%   44.49%           
=======================================
  Files          80       80           
  Lines        5558     5558           
=======================================
  Hits         2473     2473           
  Misses       2915     2915           
  Partials      170      170

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

at-wat

Seems very nice!

at-wat · 2025-05-28T08:32:13Z

.github/workflows/ci.yaml

+        working-directory: pkg/codec/ffmpeg
+        run: |
+          ls -la tmp/n7.0/
+          ls -la tmp/n7.0/lib/ || echo "lib directory not found"


false || echo will always success.
It should be like

Suggested change

ls -la tmp/n7.0/lib/ || echo "lib directory not found"

ls -la tmp/n7.0/lib/ || (echo "lib directory not found"; exit 1)

I think we can just delete this "Verify FFmpeg installation" task? I only added it for the sake of debugging CI.

Just removing this part would be fine

.github/workflows/ci.yaml

pkg/codec/ffmpeg/ffmpeg.go

at-wat · 2025-05-28T08:49:31Z

pkg/codec/ffmpeg/ffmpeg.go

+
+	for {
+		if err = e.codecCtx.ReceivePacket(e.packet); err != nil {
+			if errors.Is(err, astiav.ErrEof) || errors.Is(err, astiav.ErrEagain) {
+				continue


Will it be a busy loop or is there a sleep in ReceivePacket?

This is a busy loop, avcodec_receive_packet which is the underlying C function ReceivePacket calls returns ErrEagain immediatly if no data available.

I'm not very familiar with FFmpeg, but as far as I know this is common pattern when using libavcodec, and it shouldn't have too much negative impact on performance?

I'm also not familiar with ffmpeg library, but a tutorial for C (with many stars) seems breaking the loop if avcodec_receive_packet returned EAGAIN or EOF.

pkg/codec/ffmpeg/ffmpeg.go

at-wat · 2025-05-28T08:54:52Z

pkg/codec/ffmpeg/ffmpeg_test.go

+			// TODO: check if this is a key frame
+			// if !r.(*encoder).isKeyFrame {
+			// 	t.Fatal("Not a key frame")
+			// }


Doesn't it work for now?

The commented out code is from tests of other encoders in this project, the problem here is that isKeyFrame is not a property in the newly implemented softwareEncoder and hardwareEncoder, we only have nextIsKeyFrame here so we can't just test it like this😅. This ffmpeg encoder do supports forcing key frame though. Perhaps we can parse the nalu header of the output data to verify if it's a key frame?

JoeTurki · 2025-05-28T12:43:28Z

@3DRX Sorry for taking time, I don't want to just review the code, I want to play with it and test it, I'll test it this weekend!

Thank you so much for making this happen.

3DRX · 2025-05-28T14:20:24Z

@at-wat Thanks for reviewing! I will address them in seperate commits, for the easiness of review.

3DRX · 2025-05-28T14:22:39Z

@JoeTurki No worries! I really hope this PR can be thoroughly tested and reviewed so people can have a smoother experience using it.

3DRX · 2025-05-28T14:33:08Z

@at-wat I've resolved the conversations that I think may be resolved by follow up changes for less buzz. Feel free to reopen them if needed😊

at-wat

Sorry for the delay!

Left some comments and replies.
Also it might be cleaner to make

run: |
  make

to

run: make

.github/workflows/pkg-codec-ffmpeg-ci.yaml

at-wat · 2025-06-18T08:26:59Z

.github/workflows/ci.yaml

+        working-directory: pkg/codec/ffmpeg
+        run: |
+          ls -la tmp/n7.0/
+          ls -la tmp/n7.0/lib/ || echo "lib directory not found"


Just removing this part would be fine

.github/workflows/pkg-codec-ffmpeg-ci.yaml

at-wat · 2025-06-18T08:46:18Z

pkg/codec/ffmpeg/ffmpeg.go

+
+	for {
+		if err = e.codecCtx.ReceivePacket(e.packet); err != nil {
+			if errors.Is(err, astiav.ErrEof) || errors.Is(err, astiav.ErrEagain) {
+				continue


I'm also not familiar with ffmpeg library, but a tutorial for C (with many stars) seems breaking the loop if avcodec_receive_packet returned EAGAIN or EOF.

3DRX · 2025-06-18T17:01:27Z

@at-wat Thanks for your review! This is ffmpeg's documentation

https://ffmpeg.org/doxygen/3.3/group__lavc__encdec.html

I think we need to keep trying when received EAGAIN, but I'm not sure about EOF, I will test it later, hopefully this weekend.

at-wat · 2025-06-20T04:55:41Z

https://ffmpeg.org/doxygen/3.3/group__lavc__encdec.html

For encoding, call avcodec_receive_packet(). On success, it will return an AVPacket with a compressed frame. Repeat this call until it returns AVERROR(EAGAIN) or an error. The AVERROR(EAGAIN) return value means that new input data is required to return new output. In this case, continue with sending input. For each input frame/packet, the codec will typically return 1 output frame/packet, but it can also be 0 or more than 1.

It seems saying that we should end the loop and send a new input data if EAGAIN is returned.
Also, EOF usually means there will be no data after that so the loop should be stopped in this case as well.

3DRX · 2025-07-03T13:50:43Z

@at-wat Sorry for the delay, I've tested and tweaked the codec config options and found that as long as the encoder is configured with constrained VBR and no B frames (suitable for realtime video), every ReceivePacket call should success. Therefore we can get rid of the for loop at all, and stop on any error returned.

Thank you so much for the review :)

at-wat

Sorry for the delay!
hardwareEncoder is fixed but softwareEncoder seems not.

Also, please click Re-request review to make the updated PR listed at https://github.com/pulls/review-requested

at-wat · 2025-09-30T06:55:07Z

pkg/codec/ffmpeg/ffmpeg.go

+	for {
+		if err = e.codecCtx.ReceivePacket(e.packet); err != nil {
+			if errors.Is(err, astiav.ErrEof) || errors.Is(err, astiav.ErrEagain) {
+				continue
+			}
+			return nil, func() {}, err
+		}
+		break
+	}


This error handling should be fixed as well as the hardwareEncoder

3DRX force-pushed the add_codec_ffmpeg branch from 25768e5 to 94cbd6b Compare April 10, 2025 13:54

3DRX marked this pull request as ready for review April 10, 2025 13:56

3DRX force-pushed the add_codec_ffmpeg branch 4 times, most recently from 35848ce to d1c609a Compare April 10, 2025 16:23

3DRX requested review from Sean-Der and JoeTurki April 10, 2025 16:34

3DRX force-pushed the add_codec_ffmpeg branch 2 times, most recently from f4a451c to 39bc14a Compare April 13, 2025 13:44

3DRX mentioned this pull request Apr 15, 2025

vaapi does not support VP9 Low Power encoding #623

Closed

3DRX force-pushed the add_codec_ffmpeg branch 4 times, most recently from fc86229 to 71168b7 Compare April 16, 2025 03:59

3DRX mentioned this pull request Apr 16, 2025

vaapi does not support card1 as for encoding #622

Open

3DRX force-pushed the add_codec_ffmpeg branch 6 times, most recently from cd37985 to ac5650d Compare April 16, 2025 05:46

3DRX force-pushed the add_codec_ffmpeg branch 2 times, most recently from f741aed to 1bccd6b Compare April 23, 2025 16:19

3DRX force-pushed the add_codec_ffmpeg branch 2 times, most recently from be4ca15 to 16f049b Compare May 9, 2025 04:43

3DRX force-pushed the add_codec_ffmpeg branch from 673f782 to baea418 Compare May 24, 2025 13:30

3DRX requested a review from at-wat May 24, 2025 13:34

at-wat reviewed May 28, 2025

View reviewed changes

3DRX requested a review from at-wat June 13, 2025 06:41

at-wat reviewed Jun 18, 2025

View reviewed changes

3DRX force-pushed the add_codec_ffmpeg branch from f23ca08 to e49fdd9 Compare June 18, 2025 16:36

3DRX and others added 4 commits July 3, 2025 21:06

Add codec ffmpeg

00e120c

resolve cr issues: tidy code

32bfdbb

resolve cr issues: tidy ci

104cc5a

resolve cr issues: tidy pkg-codec-ffmpeg-ci.yaml

29b9eaf

3DRX force-pushed the add_codec_ffmpeg branch from 02d9788 to 29b9eaf Compare July 3, 2025 13:06

correct error handling and codec config

d8741c0

3DRX force-pushed the add_codec_ffmpeg branch from 4d17ebc to d8741c0 Compare July 3, 2025 13:44

at-wat self-requested a review September 26, 2025 01:10

at-wat reviewed Sep 30, 2025

View reviewed changes

	ls -la tmp/n7.0/lib/ \|\| echo "lib directory not found"
	ls -la tmp/n7.0/lib/ \|\| (echo "lib directory not found"; exit 1)

Add codec ffmpeg #621

Are you sure you want to change the base?

Add codec ffmpeg #621

Uh oh!

Conversation

3DRX commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

at-wat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeTurki commented May 28, 2025

Uh oh!

3DRX commented May 28, 2025

Uh oh!

3DRX commented May 28, 2025

Uh oh!

3DRX commented May 28, 2025

Uh oh!

at-wat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

3DRX commented Jun 18, 2025

Uh oh!

at-wat commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

3DRX commented Jul 3, 2025

Uh oh!

at-wat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

3DRX commented Apr 8, 2025 •

edited

Loading

codecov bot commented Apr 10, 2025 •

edited

Loading

at-wat commented Jun 20, 2025 •

edited

Loading