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

obs-ffmpeg: Enable multi-track audio support for mpegts #9028

Merged
merged 2 commits into from Jan 7, 2024

Conversation

pkviet
Copy link
Member

@pkviet pkviet commented Jun 5, 2023

Description

This adds support for multi-track audio for mpegts outputs (srt or rist).
Also modifies UI.
For mpegts compatible protocols, this changes radio buttons into checkboxes for the audio track selection in the Streaming output (only in Advanced output).

obs64_2023-06-05_21-27-40

Motivation and Context

YouTube has added support for SRT ingest and allowed broadcast engineers to test it during the 2023 SRT Plugfest.
I was told during the zoom introduction that multi-track support is being worked on by YouTube (public discussion,
there was no NDA).
I had this code around for a while, so it seems it's the right time to add it to obs.
Maybe multi-track will be supported also by RTMP (who knows) and the code would use the UI parts of this PR.

How Has This Been Tested?

Tested against several services during 2023 SRT Plugfest.
Works as expected. One can play any track.

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 pkviet added Seeking Testers Build artifacts on CI New Feature New feature or plugin UI/UX Anything to do with changes or additions to UI/UX elements. labels Jun 5, 2023
@GeorgesStavracas
Copy link
Member

We've reviewed this and the question that was raised is if this change prevents unselecting all audio tracks. Recently OBS started enforcing that at least one audio track is selected for recordings, and this behaviour should be reflected here too.

@pkviet
Copy link
Member Author

pkviet commented Jun 5, 2023

We've reviewed this and the question that was raised is if this change prevents unselecting all audio tracks. Recently OBS started enforcing that at least one audio track is selected for recordings, and this behaviour should be reflected here too.

I'm really not sure.
I coded that a while ago. So i might well have overlooked some new features when rebasing.
Something i'll have to think over is when in recording one decides to use the stream track.
I haven't put any thought yet on that since it's on the recording side.

@pkviet
Copy link
Member Author

pkviet commented Jun 6, 2023

We've reviewed this and the question that was raised is if this change prevents unselecting all audio tracks. Recently OBS started enforcing that at least one audio track is selected for recordings, and this behaviour should be reflected here too.

@GeorgesStavracas i checked current behaviour with recording and there's a warning indeed when no track is selected. I see now what you meant. I've added that too in case some clueless user uncheck all audio tracks.

@pkviet pkviet force-pushed the srt-track branch 2 times, most recently from 434736c to 73dd9f8 Compare June 6, 2023 17:41
@Lain-B Lain-B requested a review from tytan652 June 10, 2023 23:21
Copy link
Collaborator

@tytan652 tytan652 left a comment

Choose a reason for hiding this comment

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

I think it should not be coded as a MPEG TS only feature when it comes to naming, since in the future another container/protocol might do the same thing.

UI/window-basic-main-outputs.cpp Outdated Show resolved Hide resolved
@norihiro
Copy link
Contributor

The commit 73dd9f8 has a too long line, 80 characters, where it's limit is 72.

This enables audio multi-track support in UI for mpegts streams (srt, rist ...).

@pkviet
Copy link
Member Author

pkviet commented Jun 13, 2023

I think it should not be coded as a MPEG TS only feature when it comes to naming, since in the future another container/protocol might do the same thing.

yeah, I had already thought about it. This is code I wrote a long while ago. But given there's a new rtmp spec which is still being worked on, I did plan on updating the code to be protocol neutral. I wanted the PR out sooner than later due to SRT PlugFest.

@pkviet
Copy link
Member Author

pkviet commented Jun 13, 2023

The commit 73dd9f8 has a too long line, 80 characters, where it's limit is 72.

This enables audio multi-track support in UI for mpegts streams (srt, rist ...).

thanks, i'll fix that

@pkviet
Copy link
Member Author

pkviet commented Jun 13, 2023

i'm gonna also use dedicated stream tracks instead of the recording tracks since some people might want to change encoders for streaming and recording.

@pkviet
Copy link
Member Author

pkviet commented Jun 16, 2023

Update

  • switched to neutral UI changes to accommodate other protocols with multi-track capability (ex: whip).
  • removed the recourse to set_mixers function reserved to raw audio.
  • I'll later add support for language tags

@krakow10
Copy link

I've been using this patch in one form or another since July 2022. I can confirm that it continues to work on OBS v30.

@pkviet
Copy link
Member Author

pkviet commented Dec 17, 2023

I've been using this patch in one form or another since July 2022. I can confirm that it continues to work on OBS v30.

Well, I first have to address the comments, which I didn't because ... life.
I hope it can be merged in next point release. We'll see

@t3pmd
Copy link

t3pmd commented Dec 23, 2023

Would love to see this in the next version please. would help alot

This adds support for multiple audio tracks for the new mpegts output.

Signed-off-by: pkv <pkv@obsproject.com>
This enables audio multi-track support in UI for mpegts streams (srt,
rist ...).
The UI changes were coded though to allow re-use by other protocols.

Signed-off-by: pkv <pkv@obsproject.com>
@pkviet
Copy link
Member Author

pkviet commented Dec 24, 2023

Was so long ago, I had forgotten that I had actually addressed the comments. Will request that it moves forward.

@pkviet
Copy link
Member Author

pkviet commented Dec 24, 2023

Rebased to current master.

@pkviet pkviet added this to the OBS Studio (Next Version) milestone Dec 24, 2023
@Lain-B Lain-B merged commit f186268 into obsproject:master Jan 7, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
New Feature New feature or plugin Seeking Testers Build artifacts on CI UI/UX Anything to do with changes or additions to UI/UX elements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants