obs-webrtc: Add AAC Support #9567

troman123 · 2023-09-06T08:37:06Z

Description

according to rfc6416, This adds AAC support to obs-webrtc
https://datatracker.ietf.org/doc/rfc6416/

Motivation and Context

My motivation is to support RTP AAC in WebRTC according to RFC-6416.

How Has This Been Tested?

All testing during development was performed on macos-13.5.1 (22G90).
used aac encoder:
FFmpeg aac
coreaudio aac

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code has been run through clang-format.
I have read the contributing document.
My code is not on the master branch.
The code has been tested.
All commit messages are properly formatted and commits squashed where appropriate.
I have included updates to all appropriate documentation.

pkviet · 2023-09-06T08:50:57Z

Thanks for your contribution. There are some formal issues.

Please fill the template, especially motivation and tests.
Also run clang-format on your code.
your commit has no message. A description is mandatory.

In particular, tests with our various aac encoders would be useful:

FFmpeg aac
coreaudio
fdk-aac

You'll also have to fix the CI issues. Check the CI logs ; seems flatpak and macos do not build.

pkviet · 2023-09-06T08:56:33Z

windows CI doesn't pass either:
D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(142,17): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(142,17): error C2220: the following warning is treated as an error [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(264,4): error C2065: 'RTC_CODEC_AAC': undeclared identifier [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(271,17): error C2078: too many initializers [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(275,3): error C3861: 'rtcSetAACPacketizationHandler': identifier not found [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj]

tytan652 · 2023-09-06T09:07:07Z

Requires libdatachannel 0.19.0 stable release

pkviet · 2023-09-06T09:25:12Z

Requires libdatachannel 0.19.0 stable release

ah, aac was added to libdatachannel on august 23 indeed by the same author.
@troman123 if you want to pass CI, you'll have to point build-spec.json to your fork of obs-deps which incorporate the update of libdatachannel to 0.19 stable release (in a separate commit which will be dropped upon eventual merge)
I'd suggest you also PR the libdatachannel update to obs-deps.
Also, for flatpak, you'll have to have a commit to point to 0.19 stable.

troman123 · 2023-09-06T09:44:12Z

@pkviet , thanks for your suggestion, i will follow those steps to fix

pkviet · 2023-09-06T09:46:58Z

@pkviet , thanks for your suggestion, i will follow those steps to fix

Also fill the template and fix the commit message, as I pointed earlier.
It's not in mergeable state otherwise.

troman123 · 2023-09-06T13:22:20Z

windows CI doesn't pass either: D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(142,17): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(142,17): error C2220: the following warning is treated as an error [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(264,4): error C2065: 'RTC_CODEC_AAC': undeclared identifier [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(271,17): error C2078: too many initializers [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj] D:\a\obs-studio\obs-studio\plugins\obs-webrtc\whip-output.cpp(275,3): error C3861: 'rtcSetAACPacketizationHandler': identifier not found [D:\a\obs-studio\obs-studio\build_x64\plugins\obs-webrtc\obs-webrtc.vcxproj]

fixed

troman123 · 2023-09-07T02:42:41Z

Thanks for your contribution. There are some formal issues.

Please fill the template, especially motivation and tests.

Also run clang-format on your code.

your commit has no message. A description is mandatory.

In particular, tests with our various aac encoders would be useful:

FFmpeg aac

coreaudio

fdk-aac

You'll also have to fix the CI issues. Check the CI logs ; seems flatpak and macos do not build.

done

Sean-Der · 2024-01-29T20:57:18Z

@troman123 Would you mind rebasing this PR? Master now has the obs-deps you need!

according to rfc6416, This adds AAC support to obs-webrtc Signed-off-by: troman <[email protected]>

troman123 · 2024-05-07T11:04:51Z

@troman123 Would you mind rebasing this PR? Master now has the obs-deps you need!

Sorry, the pr conflict has been resolved now.

troman123 · 2024-05-07T11:07:02Z

@Sean-Der @RytoEX

MarcoMayCry · 2024-05-07T12:36:15Z

Tested on MacOS and works. (FFmpeg AAC / CoreAudio AAC)
More, WHIP HEVC #10159 has been tested and works. (Apple VT HEVC Hardware Encoder)

troman123 · 2024-05-24T02:59:04Z

@Sean-Der @RytoEX I have rebased the master. can be ready for merging now. thank you.

RytoEX

Quick scan. I might need someone else who understands WebRTC/WHIP to clarify and clean up the code comments.

RytoEX · 2024-08-23T18:13:05Z

plugins/obs-webrtc/whip-output.cpp

+
+		if (is_aac) {
+			/*
+			// Latm to write, StreamMuxConfig:out of band
+			// RFC6416 6.3. Fragmentation of MPEG-4 Audio Bitstream (p17)
+			// It is RECOMMENDED to put one audioMuxElement in each RTP packet. If
+			// the size of an audioMuxElement can be kept small enough that the size
+			// of the RTP packet containing it does not exceed the size of the Path
+			// MTU, this will be no problem.
+			// in this case , PayloadLengthInfo will not be writen in rtp payload.
+
+			uint8_t buf[1200] = {0};
+			// PayloadLengthInfo()
+			int header_size = packet->size/0xFF + 1;
+			memset(buf, 0xFF, header_size - 1);
+			buf[header_size - 1] = packet->size % 0xFF;
+
+			//PayloadMux
+			memcpy(buf + header_size, packet->data, packet->size);
+
+			Send(buf, header_size + packet->size, duration, audio_track);
+			*/
+		}
+


Why is this just adding a large comment block?

RytoEX · 2024-08-23T18:13:15Z

plugins/obs-webrtc/whip-output.cpp

@@ -1,6 +1,5 @@
 #include "whip-output.h"
 #include "whip-utils.h"
-


Don't remove this line.

RytoEX · 2024-08-23T18:13:57Z

plugins/obs-webrtc/whip-output.cpp

+	if (object_type == 5 /*AOT_SBR*/
+	    || (object_type == 29 /*AOT_PS*/ &&
+		!(show_bits(&gb, 3) & 0x03 && !(show_bits(&gb, 9) & 0x3F)))) {
+		if (object_type == 29 /*AOT_PS*/)
+			ps = 1;
+		ext_object_type = 5 /*AOT_SBR*/;
+		sbr = 1;
+
+		ext_sample_rate = get_sample_rate(&gb, &ext_sampling_index);
+		object_type = get_object_type(&gb);
+		if (object_type == 22 /*AOT_ER_BSAC*/)


I would rather see these object_type values as an enum.

RytoEX · 2024-08-23T18:14:31Z

plugins/obs-webrtc/whip-output.cpp

+	}
+	specific_config_bitindex = gb.bit_index;
+
+	//no support AOT_ALS


Suggested change

//no support AOT_ALS

// no support AOT_ALS

Sean-Der · 2024-08-23T18:20:06Z

Sorry I didn't look at this sooner!

Would it be possible to put non-OBS specific code upstream? Other projects could benefit from this, and want to keep the OBS+WebRTC code maintainable.

I am 100% in support of AAC though. Anything that makes it easier for users.

pkviet added Seeking Testers Build artifacts on CI New Feature New feature or plugin labels Sep 6, 2023

derrod marked this pull request as draft September 6, 2023 08:59

pkviet removed the Seeking Testers Build artifacts on CI label Sep 6, 2023

tytan652 added Compile Issue There are issues getting this PR compiled Dependency Issue Issue is in external library or dependency, not OBS itself labels Sep 6, 2023

troman123 force-pushed the aac branch from ba9aaa8 to 1c47c88 Compare September 6, 2023 09:20

troman123 force-pushed the aac branch 3 times, most recently from db3d477 to fe69dc1 Compare September 6, 2023 12:44

troman123 closed this Sep 6, 2023

troman123 reopened this Sep 6, 2023

troman123 marked this pull request as ready for review September 7, 2023 02:29

troman123 force-pushed the aac branch from fe69dc1 to 90987cf Compare September 7, 2023 02:41

troman123 marked this pull request as draft September 7, 2023 05:06

RytoEX mentioned this pull request Jan 27, 2024

CI: Update deps to 2024-01-27 release #10150

Merged

6 tasks

troman123 marked this pull request as ready for review May 7, 2024 09:46

troman123 force-pushed the aac branch from 2701224 to 1757171 Compare May 7, 2024 09:50

obs-webrtc: Add AAC Support

e5c1e2f

according to rfc6416, This adds AAC support to obs-webrtc Signed-off-by: troman <[email protected]>

troman123 force-pushed the aac branch from 1757171 to e5c1e2f Compare May 7, 2024 10:09

RytoEX removed Compile Issue There are issues getting this PR compiled Dependency Issue Issue is in external library or dependency, not OBS itself labels Aug 23, 2024

RytoEX requested changes Aug 23, 2024

View reviewed changes

		@@ -1,6 +1,5 @@
		#include "whip-output.h"
		#include "whip-utils.h"

Uh oh!

obs-webrtc: Add AAC Support #9567

Are you sure you want to change the base?

obs-webrtc: Add AAC Support #9567

Uh oh!

Conversation

troman123 commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

pkviet commented Sep 6, 2023

Uh oh!

pkviet commented Sep 6, 2023

Uh oh!

tytan652 commented Sep 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkviet commented Sep 6, 2023

Uh oh!

troman123 commented Sep 6, 2023

Uh oh!

pkviet commented Sep 6, 2023

Uh oh!

troman123 commented Sep 6, 2023

Uh oh!

troman123 commented Sep 7, 2023

Uh oh!

Sean-Der commented Jan 29, 2024

Uh oh!

troman123 commented May 7, 2024

Uh oh!

troman123 commented May 7, 2024

Uh oh!

MarcoMayCry commented May 7, 2024

Uh oh!

troman123 commented May 24, 2024

Uh oh!

RytoEX left a comment

Choose a reason for hiding this comment

Uh oh!

RytoEX Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

RytoEX Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

RytoEX Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

RytoEX Aug 23, 2024

Choose a reason for hiding this comment

Uh oh!

Sean-Der commented Aug 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

troman123 commented Sep 6, 2023 •

edited

Loading

tytan652 commented Sep 6, 2023 •

edited

Loading

Sean-Der commented Aug 23, 2024 •

edited

Loading