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.

Sign up for GitHub

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

Jump to bottom

obs-outputs: Add eRTMP multitrack audio support #10618

Merged
merged 5 commits into from May 24, 2024
Merged

obs-outputs: Add eRTMP multitrack audio support #10618

merged 5 commits into from May 24, 2024
+235 −303

Conversation

palana
Copy link
Contributor

@palana palana commented Apr 29, 2024 •
edited

Description

In addition to multitrack video, eRTMP also includes support for multitrack audio, thus making things like VOD track available in the spec, rather than relying on FLV byte arrays

Motivation and Context

VOD track currently uses FLV byte arrays to carry the additional audio track, where byte arrays are part of the spec, but using them for audio isn't. With the updated eRTMP multitrack audio support general overhead is lower and also the transport format is part of an open specification.

How Has This Been Tested?

eRTMP multitrack audio is being tested internally at Twitch

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.

@WizardCM WizardCM added the New Feature New feature or plugin label Apr 29, 2024
@RytoEX RytoEX self-assigned this Apr 30, 2024
@palana palana force-pushed the ruwen/upstream-multitrack-audio branch 2 times, most recently from 549d4c1 to f1b0094 Compare May 3, 2024 14:23
@RytoEX
Copy link
Member

RytoEX commented May 3, 2024

#10500 has been merged, so this can be rebased and undrafted.

RytoEX
RytoEX requested changes May 3, 2024
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Commit message nit:

obs-outputs: Add erRTMP multitrack audio support

erRTMP -> eRTMP

@palana palana marked this pull request as ready for review May 6, 2024 12:34
@palana palana force-pushed the ruwen/upstream-multitrack-audio branch 2 times, most recently from 28e0352 to b55ac21 Compare May 7, 2024 10:37
@RytoEX RytoEX requested review from derrod and RytoEX May 8, 2024 15:39
@RytoEX RytoEX force-pushed the ruwen/upstream-multitrack-audio branch from b55ac21 to 2587c32 Compare May 8, 2024 18:00
@RytoEX
Copy link
Member

RytoEX commented May 8, 2024

For what it's worth, I tested this and it worked as expected/configured.

RytoEX
RytoEX approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

Tentatively approving this. Tests seemed fine. Would appreciate a second review.

@RytoEX
Copy link
Member

RytoEX commented May 14, 2024

Holding this due to a confirmed issue regarding audio/video desync that is unique to this implementation.

@RytoEX RytoEX marked this pull request as draft May 15, 2024 00:00
@koolscooby
Copy link

Adding the link to the public alpha Enhanced RTMP v2 spec for posterity.

@palana
Copy link
Contributor Author

palana commented May 21, 2024

Holding this due to a confirmed issue regarding audio/video desync that is unique to this implementation.

@RytoEX: I just pushed a fix for the desync, it was an issue in librtmp's handling of type 3 chunks -> undrafting

@palana palana marked this pull request as ready for review May 21, 2024 11:50
@palana palana force-pushed the ruwen/upstream-multitrack-audio branch from 12635f2 to 9f2a2ef Compare May 21, 2024 11:51
@Lain-B
Copy link
Collaborator

Lain-B commented May 21, 2024 •
edited

Great work tracking that multitrack bug down. That must have been a pain.

@RytoEX
Copy link
Member

RytoEX commented May 21, 2024

Not sure if the librtmp commit should be first or not. I wouldn't want the eRTMP multitrack audio commit to exhibit the buggy behavior, but I wasn't sure if the librtmp commit has other effects on the existing VOD Audio Track implementation.

Lain-B
Lain-B approved these changes May 22, 2024
View reviewed changes
Copy link
Collaborator

@Lain-B Lain-B left a comment

Choose a reason for hiding this comment

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

Great work. Ryan's suggestion to put the commit at the beginning rather than the end might be good but it doesn't really prevent merging of this PR or anything. We'll wait for just a bit more verification/testing to make sure everything's okay but otherwise seems good to go.

@palana
Copy link
Contributor Author

palana commented May 22, 2024

Not sure if the librtmp commit should be first or not. I wouldn't want the eRTMP multitrack audio commit to exhibit the buggy behavior, but I wasn't sure if the librtmp commit has other effects on the existing VOD Audio Track implementation.

agreed, will move it to the beginning

it doesn't affect the current vod track (as far as I can tell) because the current vod track uses RTMP_PACKET_TYPE_INFO 1
which always ends up as RTMP_PACKET_SIZE_LARGE 2 and RTMP_PACKET_SIZE_LARGE always carries an absolute timestamp and is never being considered for header compression 3

Footnotes

  1. https://github.com/obsproject/obs-studio/blob/27fa9b1eedbed06753b8d1e090aa760987ff3e46/plugins/obs-outputs/flv-mux.c#L787 ↩

  2. https://github.com/obsproject/obs-studio/blob/27fa9b1eedbed06753b8d1e090aa760987ff3e46/plugins/obs-outputs/librtmp/rtmp.c#L5344-L5346 ↩

  3. https://github.com/obsproject/obs-studio/blob/27fa9b1eedbed06753b8d1e090aa760987ff3e46/plugins/obs-outputs/librtmp/rtmp.c#L4118 ↩

@palana palana force-pushed the ruwen/upstream-multitrack-audio branch from 9f2a2ef to 45fbcb2 Compare May 22, 2024 10:54
@palana
librtmp: Fix incorrect usage of type 3 chunks
fbf3168 
Per [1] type 3 chunks/RTMP_PACKET_SIZE_MINIMUM always use
the previously sent (delta) timestamp as their _delta_ timestamp,
so we need to inspect whatever was previously sent, rather than just
looking at the previous packet's absolute timestamp. I.e., type 3
chunks are only permissible in this case if the previously encoded
(delta) timestamp equals the current delta timestamp.

[1] https://rtmp.veriskope.com/docs/spec/#53124-type-3
@palana
obs-outputs: Add eFLV multitrack audio support
3d77161
@palana
obs-outputs: Add eRTMP multitrack audio support
6606745
@palana
obs-outputs: Remove special handling for audio encoders 1 and 2
2b206ab
@palana palana force-pushed the ruwen/upstream-multitrack-audio branch from 45fbcb2 to f080c6b Compare May 22, 2024 10:55
@RytoEX
Copy link
Member

RytoEX commented May 24, 2024

Further tests with the added librtmp commit seem promising, so I'm going to call this safe and merge it.

@RytoEX RytoEX added this to the OBS Studio (Next Version) milestone May 24, 2024
@RytoEX RytoEX merged commit 52660be into obsproject:master May 24, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RytoEX RytoEX RytoEX approved these changes

@Lain-B Lain-B Lain-B approved these changes

@derrod derrod Awaiting requested review from derrod

Assignees

@RytoEX RytoEX

Labels
New Feature New feature or plugin
Projects
None yet
Milestone
OBS Studio (Next Version)
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@palana @RytoEX @koolscooby @Lain-B @derrod @WizardCM