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

[Feature] Add H265 support with SDP, RTP payloader-deplayloader #1965

Merged
merged 35 commits into from
May 1, 2024
Merged

[Feature] Add H265 support with SDP, RTP payloader-deplayloader #1965

merged 35 commits into from
May 1, 2024

Conversation

niyatim23
Copy link
Contributor

@niyatim23 niyatim23 commented Apr 7, 2024 •
edited
Loading

Issue #, if available:

What was changed?

  • Added support for H265 in RTP and SDP

Why was it changed?

  • This is new feature to expand support for different codecs

How was it changed?

  • Introduced H265 payloader and de-payloader in RTP
  • Modified the SessionDescription.c to include H265 in offer and answer

What testing was done for the changes?

Desktop % ffprobe -v error -show_entries stream=codec_name,sample_rate,width,height,channels -of default=noprint_wrappers=1 video.mkv
codec_name=hevc
width=1280
height=720
codec_name=opus
sample_rate=48000
channels=2
niyatim@6c7e67b7ca72 Desktop %

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@niyatim23 niyatim23 added the enhancement New feature or request label Apr 7, 2024
@niyatim23 niyatim23 marked this pull request as ready for review April 8, 2024 15:31
@codecov-commenter
Copy link

codecov-commenter commented Apr 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.06787% with 33 lines in your changes are missing coverage. Please review.

Project coverage is 88.61%. Comparing base (b3522a6) to head (c069312).
Report is 1 commits behind head on develop.

❗ Current head c069312 differs from pull request most recent head ca9695a. Consider uploading reports for the commit ca9695a to get more accurate results

Files Patch % Lines
src/source/PeerConnection/SessionDescription.c 55.00% 18 Missing ⚠️
src/source/PeerConnection/PeerConnection.c 64.70% 6 Missing ⚠️
src/source/Rtp/Codecs/RtpH265Payloader.c 96.81% 5 Missing ⚠️
src/source/PeerConnection/Rtp.c 0.00% 4 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #1965      +/-   ##
===========================================
- Coverage    88.78%   88.61%   -0.17%     
===========================================
  Files           46       47       +1     
  Lines        12476    12695     +219     
===========================================
+ Hits         11077    11250     +173     
- Misses        1399     1445      +46

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

hassanctech
hassanctech reviewed Apr 18, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
hassanctech
hassanctech reviewed Apr 19, 2024
View reviewed changes
Copy link
Contributor

@hassanctech hassanctech left a comment

Choose a reason for hiding this comment

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

In the PR description it says it was tested with h265, I do not see any way to tell the master sample to use h265 it has hard coded h264 option. Are those changes on a different branch somewhere else? I do not see them in develop either.

@niyatim23 niyatim23 changed the title [Feature] Add H265 Support [Feature] Add H265 Support with SDP, RTP payloader-deplayloader Apr 23, 2024
@niyatim23 niyatim23 changed the title [Feature] Add H265 Support with SDP, RTP payloader-deplayloader [Feature] Add H265 support with SDP, RTP payloader-deplayloader Apr 23, 2024
@niyatim23
Copy link
Contributor Author

niyatim23 commented Apr 25, 2024 •
edited
Loading

In the PR description it says it was tested with h265, I do not see any way to tell the master sample to use h265 it has hard coded h264 option. Are those changes on a different branch somewhere else? I do not see them in develop either.

I've added a command-line argument and updated the readme with new instructions. H264 and Opus are still the default.

disa6302
disa6302 reviewed Apr 29, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
samples/Common.c Outdated Show resolved Hide resolved
samples/kvsWebRTCClientMaster.c Show resolved Hide resolved
samples/kvsWebRTCClientMaster.c Show resolved Hide resolved
samples/kvsWebRTCClientMasterGstSample.c Outdated Show resolved Hide resolved
samples/kvsWebRTCClientMasterGstSample.c Outdated Show resolved Hide resolved
src/source/Rtp/Codecs/RtpH264Payloader.c Show resolved Hide resolved
src/source/Rtp/Codecs/RtpH265Payloader.h Show resolved Hide resolved
hassanctech
hassanctech reviewed Apr 30, 2024
View reviewed changes
README.md Show resolved Hide resolved
hassanctech
hassanctech reviewed May 1, 2024
View reviewed changes
Copy link
Contributor

@hassanctech hassanctech left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A general remark so different encoders (hardware/software) may have different quirks. Aside from referencing the RFCs and following what should be supported, it is really important we look at a variety of encoders that are actually out in the wild and make sure this code is robust enough to work with them. At a minimum we should test with hardware and software encoders on iOS/Android and libx265 software encoder. If this PR could include a matrix of encoders / combinations that were tested we can know in the future we are not regressing and we will know and have documented what gaps we have.

hassanctech
hassanctech approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@hassanctech hassanctech left a comment

Choose a reason for hiding this comment

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

I haven't looked at the RFC myself to finely go through the payloader/depayloader code will assume the proper due diligence was done there.

disa6302
disa6302 approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@disa6302 disa6302 left a comment

Choose a reason for hiding this comment

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

LGTM.

@niyatim23
Copy link
Contributor Author

niyatim23 commented May 1, 2024 •
edited
Loading

A general remark so different encoders (hardware/software) may have different quirks. Aside from referencing the RFCs and following what should be supported, it is really important we look at a variety of encoders that are actually out in the wild and make sure this code is robust enough to work with them. At a minimum we should test with hardware and software encoders on iOS/Android and libx265 software encoder. If this PR could include a matrix of encoders / combinations that were tested we can know in the future we are not regressing and we will know and have documented what gaps we have.

Tested the following combinations

Master Viewer
kvsWebrtcClientMaster Safari Version 17.4.1
kvsWebrtcClientMaster Android phone (Samsung SM-S908U Android 12) with Sample with H265 libs here
kvsWebrtcClientMaster kvsWebrtcClientViewerGstSample (ffplay with file obtained from filesink)
kvsWebrtcClientMasterGstSample Safari Version 17.4.1
kvsWebrtcClientMasterGstSample Android phone (Samsung SM-S908U Android 12) with Sample with H265 libs here
kvsWebrtcClientMasterGstSample kvsWebrtcClientViewerGstSample (ffplay with file obtained from filesink)
Safari Version 17.4.1 kvsWebrtcClientViewerGstSample (ffplay with file obtained from filesink)

@niyatim23 niyatim23 merged commit a28e1e4 into develop May 1, 2024
31 checks passed
@niyatim23 niyatim23 deleted the h265-support branch May 1, 2024 22:30
niyatim23 added a commit that referenced this pull request May 1, 2024
@niyatim23 @hongliwo
[Feature] Add H265 support with SDP, RTP payloader-deplayloader (#1965)
44f307f 
* Add RtpH265Payloader.c and RtpH265Payloader.h

* Add support for H265 in PeerConnection/SessionDescription

* Add support for H265 in PeerConnection/PeerConnection

* Add support for H265 in PeerConnection/Rtp

* Add support for H265 in samples/Common.c

* Add support for H265 in samples/kvsWebRTCClientMaster.c

* rtp, sdp fix, flag removed, clang fixed, windows build fixed, new test added

* test fix

* cleanup

* cleanup

* remove #if 0

* clang

* presentation ts fix

* clang-format fix

* PKG_CONFIG_PATH in kvscommon

* missing bracket

* fix all builds

* ci

* cleanup

* fix windows build, rename h265 defs

* remove duplicate line from h264 and h265

* sample changes

* address comments

* clang-format

* gst sample

* cleanup args

* clang-format

* cleanup

* add sdp tests

* address comments

* address commentas

* set default payload type only once

* address comments

* fix height and width

* sdp change

---------

Co-authored-by: Hongli Wang <[email protected]>
@niyatim23 niyatim23 mentioned this pull request May 1, 2024
Closed
@niyatim23 niyatim23 restored the h265-support branch May 2, 2024 12:26
disa6302 pushed a commit that referenced this pull request May 2, 2024
@niyatim23 @hongliwo @disa6302
[Feature] Add H265 support with SDP, RTP payloader-deplayloader (#1965)
aa3e609 
* Add RtpH265Payloader.c and RtpH265Payloader.h

* Add support for H265 in PeerConnection/SessionDescription

* Add support for H265 in PeerConnection/PeerConnection

* Add support for H265 in PeerConnection/Rtp

* Add support for H265 in samples/Common.c

* Add support for H265 in samples/kvsWebRTCClientMaster.c

* rtp, sdp fix, flag removed, clang fixed, windows build fixed, new test added

* test fix

* cleanup

* cleanup

* remove #if 0

* clang

* presentation ts fix

* clang-format fix

* PKG_CONFIG_PATH in kvscommon

* missing bracket

* fix all builds

* ci

* cleanup

* fix windows build, rename h265 defs

* remove duplicate line from h264 and h265

* sample changes

* address comments

* clang-format

* gst sample

* cleanup args

* clang-format

* cleanup

* add sdp tests

* address comments

* address commentas

* set default payload type only once

* address comments

* fix height and width

* sdp change

---------

Co-authored-by: Hongli Wang <[email protected]>
niyatim23 added a commit that referenced this pull request May 2, 2024
@niyatim23 @hongliwo
[Feature] Add H265 support with SDP, RTP payloader-deplayloader (#1965)
9933cf7 
* Add RtpH265Payloader.c and RtpH265Payloader.h

* Add support for H265 in PeerConnection/SessionDescription

* Add support for H265 in PeerConnection/PeerConnection

* Add support for H265 in PeerConnection/Rtp

* Add support for H265 in samples/Common.c

* Add support for H265 in samples/kvsWebRTCClientMaster.c

* rtp, sdp fix, flag removed, clang fixed, windows build fixed, new test added

* test fix

* cleanup

* cleanup

* remove #if 0

* clang

* presentation ts fix

* clang-format fix

* PKG_CONFIG_PATH in kvscommon

* missing bracket

* fix all builds

* ci

* cleanup

* fix windows build, rename h265 defs

* remove duplicate line from h264 and h265

* sample changes

* address comments

* clang-format

* gst sample

* cleanup args

* clang-format

* cleanup

* add sdp tests

* address comments

* address commentas

* set default payload type only once

* address comments

* fix height and width

* sdp change

---------

Co-authored-by: Hongli Wang <[email protected]>
@niyatim23 niyatim23 mentioned this pull request May 2, 2024
Merged
niyatim23 added a commit that referenced this pull request May 9, 2024
@niyatim23 @disa6302 @hongliwo
Release 1.10.2 (#1981)
4f1d3ad 
* Add H265 and AAC frames (#1962)

* aac and h265 frames

* update readme

* update cmake with h265 and aac

* clang formay

* [Sample] GStreamer media receiver (#1963)

* create the gst receiver

* fix ci

* cmake flag

* clang format, add cmake flag in ci

* install gstreamer on ci

* use macos12

* use defined values

* indent

* clang-format

* readme and commentsa

* clang :(

* install glib to fix ci

* -DCOMPILER_WARNINGS=FALSE to fix the CI

* fix macos version for clang

* address comments and cleanup

* new viewer with gst

* clang-format

* missing null check

* address comments

* -DCOMPILER_WARNINGS=TRUE

* clang-format

* gst_init_check

* fix CI

* fix CI

* [Bug] GStreamer sample (#1972)

* gst-sample-fix

* modify the message

* fix windows

* set to null

* fix pts and eos in sample

* unused var removed

* move null init

* Version check job (#1974)

* Version check job

* Working Version check template

* Retest with dev

* Version lint

* Version lint

* use new frames (#1978)

* Sample TWCC implementation (#1957)

* encoder bitrate change based on twcc

* Change to 5% inc and dec

* modify percentages

* ema based calc

* EMa fix

* Nits

* Readme

* flip

* memset remove

* Readme update, move enable flags to createSampleConfiguration

* Add codecov token

* [Feature] Add H265 support with SDP, RTP payloader-deplayloader (#1965)

* Add RtpH265Payloader.c and RtpH265Payloader.h

* Add support for H265 in PeerConnection/SessionDescription

* Add support for H265 in PeerConnection/PeerConnection

* Add support for H265 in PeerConnection/Rtp

* Add support for H265 in samples/Common.c

* Add support for H265 in samples/kvsWebRTCClientMaster.c

* rtp, sdp fix, flag removed, clang fixed, windows build fixed, new test added

* test fix

* cleanup

* cleanup

* remove #if 0

* clang

* presentation ts fix

* clang-format fix

* PKG_CONFIG_PATH in kvscommon

* missing bracket

* fix all builds

* ci

* cleanup

* fix windows build, rename h265 defs

* remove duplicate line from h264 and h265

* sample changes

* address comments

* clang-format

* gst sample

* cleanup args

* clang-format

* cleanup

* add sdp tests

* address comments

* address commentas

* set default payload type only once

* address comments

* fix height and width

* sdp change

---------

Co-authored-by: Hongli Wang <[email protected]>

* Version bump, fix tests, remove enableIceStats

* Remove AAC related changes

* Remove AAC frames

* Clang format

* fix kvsWebrtcClientMasterGstSample.c name in CMakeLists

---------

Co-authored-by: Divya Sampath Kumar <[email protected]>
Co-authored-by: Hongli Wang <[email protected]>
@sirknightj sirknightj deleted the h265-support branch July 18, 2024 16:39
@sirknightj sirknightj mentioned this pull request Jul 20, 2024
Closed
3 tasks
@naseebpanghal
Copy link

Hi @niyatim23,
Is it possible to run kvsWebrtcClientMaster kvsWebrtcClientViewerGstSample both on same machine locally?

@niyatim23
Copy link
Contributor Author

Is it possible to run kvsWebrtcClientMaster kvsWebrtcClientViewerGstSample both on same machine locally?

@naseebpanghal, yes it should be

@naseebpanghal
Copy link

Hi @niyatim23
I am trying but not able to stream. I doing all this to test H265 stream which is not supported by various web browers so far. That is the reason I have chosen kvsWebrtcClientViewerGstSample as client(mentioned by you above in this post).

client.log
server.log

First i tried with H264, it works with web browser but not with local kvsWebrtcClientViewerGstSample running on same machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@disa6302 disa6302 disa6302 approved these changes

@hassanctech hassanctech hassanctech approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@niyatim23 @codecov-commenter @naseebpanghal @hassanctech @disa6302 @hongliwo