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

Add H265 payloader and depacketizer #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add H265 payloader and depacketizer #165

wants to merge 1 commit into from

Conversation

kevmo314
Copy link
Contributor

@kevmo314 kevmo314 commented Feb 9, 2022

This change completes the H265 implementation.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Attention: Patch coverage is 35.22727% with 114 lines in your changes missing coverage. Please review.

Project coverage is 79.86%. Comparing base (a21194e) to head (dd05d22).

Files with missing lines Patch % Lines
codecs/h265_packet.go 35.22% 111 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #165      +/-   ##
==========================================
- Coverage   83.95%   79.86%   -4.09%     
==========================================
  Files          24       24              
  Lines        1957     2126     +169     
==========================================
+ Hits         1643     1698      +55     
- Misses        255      366     +111     
- Partials       59       62       +3     
Flag Coverage Δ
go 79.86% <35.22%> (-4.09%) ⬇️
wasm 79.30% <35.22%> (-4.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

tubzby added a commit to tubzby/rtp that referenced this pull request Nov 14, 2023
@lebedyncrs
Copy link

Hi @kevmo314 are you planning to continue on this one?

@kevmo314
Copy link
Contributor Author

kevmo314 commented Sep 9, 2024

Hi @kevmo314 are you planning to continue on this one?

There is no additional work necessary, however there is perhaps not a huge desire to merge it yet due to the relative rarity of H265 in WebRTC. I'll defer to @Sean-Der.

This change completes the H265 implementation.
@Sean-Der
Copy link
Member

@kevmo314 @lebedyncrs Lets get this merged today! The only issue for me is Payload has no coverage

@kevmo314 Do you have any tests you liked? Or should I just generate stuff via ffmpeg. Do you have any suggestions on flags I should use?

@kevmo314
Copy link
Contributor Author

@kevmo314 @lebedyncrs Lets get this merged today! The only issue for me is Payload has no coverage

@kevmo314 Do you have any tests you liked? Or should I just generate stuff via ffmpeg. Do you have any suggestions on flags I should use?

ffmpeg-generated stuff works! The only gotcha I know of is the Safari H265 payloading thing: AlexxIT/Blog#5

I vaguely recall a post indicating that Safari has or intends to migrate off this custom format but I'm not sure how we want to handle that from pion since that's not truly spec-compliant. Pion to pion worked great for me when I had used this code.

@lebedyncrs
Copy link

lebedyncrs commented Sep 10, 2024

Thanks for pushing it forward @Sean-Der . @kevmo314 I tried to prepare example of how to stream video using h265 in browser (there is Chromium build which supports h265). I am getting an error "unable to start track, codec is not supported by remote". When I look at SDP from browser there is information about h265 support but when I check go answer SDP I cannot see it.

Steps to reproduce:

@kevmo314
Copy link
Contributor Author

Thanks for pushing it forward @Sean-Der . @kevmo314 I tried to prepare example of how to stream video using h265 in browser (there is Chromium build which supports h265). I am getting an error "unable to start track, codec is not supported by remote". When I look at SDP from browser there is information about h265 support but when I check go answer SDP I cannot see it.

Steps to reproduce:

This PR only adds the payloader and depacketizer. You'll have to configure Pion to respond with H265 which is not on by default. It can be added in the media engine.

@aleksitto-gh
Copy link

thanks for this PR, but before you merge, what about

annexbNALUStartCode ?

it looks like a mock at the moment

@kevmo314
Copy link
Contributor Author

kevmo314 commented Oct 1, 2024

thanks for this PR, but before you merge, what about

annexbNALUStartCode ?

it looks like a mock at the moment

Where do you see that it's a mock?

@aleksitto-gh
Copy link

aleksitto-gh commented Oct 1, 2024

thanks for this PR, but before you merge, what about
annexbNALUStartCode ?
it looks like a mock at the moment

Where do you see that it's a mock?

// TODO: this is not actually correct following B.2.2, not all NALUs have a 4-byte start code.
buf = append(buf, annexbNALUStartCode...)

@kevmo314
Copy link
Contributor Author

kevmo314 commented Oct 1, 2024

thanks for this PR, but before you merge, what about
annexbNALUStartCode ?
it looks like a mock at the moment

Where do you see that it's a mock?

// TODO: this is not actually correct following B.2.2, not all NALUs have a 4-byte start code.
buf = append(buf, annexbNALUStartCode...)

That doesn't mean it's a mock, it only means that it's not fully correct as the comment suggests. Pion always uses a 4-byte start code which is suboptimal according to the specification but it does work and play back correctly.

@aleksitto-gh
Copy link

thanks for this PR, but before you merge, what about
annexbNALUStartCode ?
it looks like a mock at the moment

Where do you see that it's a mock?

// TODO: this is not actually correct following B.2.2, not all NALUs have a 4-byte start code.
buf = append(buf, annexbNALUStartCode...)

That doesn't mean it's a mock, it only means that it's not fully correct as the comment suggests. Pion always uses a 4-byte start code which is suboptimal according to the specification but it does work and play back correctly.

Great, thank you! The TODO comment confused me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants