-
Notifications
You must be signed in to change notification settings - Fork 489
Fix HEVC NAL parsing and RTP de/packetization #1394
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
Conversation
put some log comments
|
@ha-ves this PR does not build for me.
|
|
Also how have you been testing H265? I've attempted to use the WebRTCFFmpegGetStarted with this PR and the corresponding SIPSorceryMedia.FFmpeg one but the codec does not get recognised.
|
|
@sipsorcery Sorry, leftover stats logging. All examples and tests with browser peers are pending since they don't support HEVC/H.265, so there will be no correctly negotiated format. I was testing in WPF. I could add it as an example with this PR, I'll have to clean it up a bit. var it = new FFmpegFileSource("max_intro.mp4", true, null);
var ti = new FFmpegVideoEndPoint();
///...
// assign endpoint event to copy to wiretablebitmap backbuffer, etc.
///...
it.RestrictFormats(f => f.Codec == VideoCodecsEnum.H265);
it.SetVideoSourceFormat(it.GetVideoSourceFormats().First());
/// ...
// create 2 PC,
// assign events, add tracks, signaling, etc.,
/// ...
while (pcrmt.connectionState != RTCPeerConnectionState.connected
|| pclcl.connectionState != RTCPeerConnectionState.connected)
Thread.Sleep(1000);
it.StartVideo(); |
|
"All examples and tests with browser peers are pending since they don't support HEVC/H.265" |
|
As @jimm98y says Chrome has announced support from version 136. I was on v135 but a pending update got me to v136. I then did try and test H265 again but it seems my GPU does not have a H265 codec. My iPhone does advertise H265 but it's a bit tricky to test on with my network set up. Given H265 support is still brand new in most cases I think it's safe enough to merge this PR. That will make it easier to test and add additional fixes if needs be. |
| if (nals.Where(x => x.NAL.Length < RTPSession.RTP_MAX_PAYLOAD).Count() > 1) | ||
| { | ||
| //logger.LogTrace("(ou) Trying aggregating {nals} nals", nals.Count()); | ||
| nals = H265Packetiser.CreateAggregated(nals, RTPSession.RTP_MAX_PAYLOAD); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any will only cause the nals sequence to be iterated until the first is found, while Count will cause the nals sequence to be completely iterated.
Both incur in delegate allocation (although in the latest SDKs this may be a static delegate instance) and invocation.
I would prefer something like:
foreach (var nal in nals)
{
if (nal.NAL.Length< RTPSession.RTP_MAX_PAYLOAD)
{
nals = H265Packetiser.CreateAggregated(nals, RTPSession.RTP_MAX_PAYLOAD);
break;
}
}
I read a bit RFC 7798 Section 4.4
Summary:
zeroesback to 0,fixes #1392
forwards sipsorcery-org/SIPSorceryMedia.FFmpeg#97