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

feat: Add TS transmuxer #5386

Merged
merged 23 commits into from
Aug 29, 2023
Merged

feat: Add TS transmuxer #5386

merged 23 commits into from
Aug 29, 2023

Conversation

avelad
Copy link
Member

@avelad avelad commented Jul 1, 2023 •
edited
Loading

  • Allow to have multiple transmuxers, but choose only the one that is valid depending on whether it supports the codecs or not
  • Supported codecs: AAC, AC-3, EC-3, H.264, MPEG/MP3
  • It is prepared to allow more codecs in the future (HEVC/H.265) (no muxed)

@avelad avelad marked this pull request as draft July 1, 2023 21:12
@avelad avelad added type: enhancement New feature or request component: HLS The issue involves Apple's HLS manifest format priority: P2 Smaller impact or easy workaround priority: P3 Useful but not urgent and removed priority: P2 Smaller impact or easy workaround labels Jul 1, 2023
@avelad avelad added this to the v4.4 milestone Jul 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 1, 2023 •
edited
Loading

Incremental code coverage: 75.92%

@avelad avelad force-pushed the ts-transmuxer branch 2 times, most recently from 0008bd7 to 806ff2e Compare July 2, 2023 17:37
@avelad avelad marked this pull request as ready for review July 2, 2023 17:41
@avelad
Copy link
Member Author

avelad commented Jul 2, 2023

I did not find any public streams with MPEG-TS and AC-3 or EC-3. But I tested it in private.

@avelad avelad force-pushed the ts-transmuxer branch 2 times, most recently from 96cceca to 4a4d84f Compare July 2, 2023 18:42
@avelad avelad requested review from joeyparrish and theodab July 2, 2023 20:57
@avelad avelad force-pushed the ts-transmuxer branch 4 times, most recently from 6029e23 to 6aae2bc Compare July 4, 2023 10:58
@joeyparrish
Copy link
Member

We need to both review this and investigate test failures on Safari.

@joeyparrish
Copy link
Member

The Safari and ChromeMac failures look new, but the Tizen ones look like the same flaky tests we have yet to resolve in main.

@joeyparrish
Copy link
Member

It's interesting that it doesn't happen on GitHub's Mac VMs, though.

@avelad
Copy link
Member Author

avelad commented Aug 21, 2023

@joeyparrish @theodab ready for review!

@joeyparrish
Copy link
Member

Awesome! I'll look at this today if I can find time, tomorrow otherwise.

joeyparrish
joeyparrish requested changes Aug 28, 2023
View reviewed changes
test/transmuxer/transmuxer_engine_integration.js Show resolved Hide resolved
lib/transmuxer/h264.js Outdated Show resolved Hide resolved
lib/transmuxer/mpeg_ts_transmuxer.js Show resolved Hide resolved
lib/transmuxer/transmuxer_engine.js Outdated Show resolved Hide resolved
lib/transmuxer/ts_transmuxer.js Show resolved Hide resolved
lib/util/ts_parser.js Show resolved Hide resolved
@avelad avelad requested a review from joeyparrish August 28, 2023 08:41
@avelad
Copy link
Member Author

avelad commented Aug 29, 2023

@joeyparrish I updated the PR to include the test for MP3 in TS

@avelad
Copy link
Member Author

avelad commented Aug 29, 2023

@joeyparrish I updated the PR to include the test for AC-3/EC-3 in TS

joeyparrish
joeyparrish requested changes Aug 29, 2023
View reviewed changes
lib/util/ts_parser.js
pts: null,
dts: null,
};

// If it is video, we expect the packet length to be 0. When the PES packet
Copy link
Member

Choose a reason for hiding this comment

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

Is an explicit packet length invalid for video? Why wouldn't you just account for an explicit packet length in subarray() below?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a design issue, because there are video nalus packages that may overlap with other PES.

lib/transmuxer/mpeg_ts_transmuxer.js Outdated Show resolved Hide resolved
lib/transmuxer/mpeg_ts_transmuxer.js Show resolved Hide resolved
lib/util/ts_parser.js Show resolved Hide resolved
lib/util/ts_parser.js Show resolved Hide resolved
@avelad avelad requested a review from joeyparrish August 29, 2023 17:52
@joeyparrish joeyparrish merged commit eec25b2 into shaka-project:main Aug 29, 2023
@avelad avelad deleted the ts-transmuxer branch August 30, 2023 06:43
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Oct 28, 2023
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Oct 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish approved these changes

@theodab theodab Awaiting requested review from theodab

Assignees
No one assigned
Labels
component: HLS The issue involves Apple's HLS manifest format priority: P3 Useful but not urgent status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.
3 participants
@avelad @joeyparrish @shaka-bot