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

Bugfix: HEVC SRT stream supports multiple PPS fields. v6.0.76 #3722

Merged
merged 12 commits into from
Sep 18, 2023
Merged

Bugfix: HEVC SRT stream supports multiple PPS fields. v6.0.76 #3722

merged 12 commits into from
Sep 18, 2023

Conversation

xengine-qyt
Copy link
Contributor

@xengine-qyt xengine-qyt commented Jul 28, 2023 •
edited by winlinvip
Loading

Please describe the summary for this PR.

when the srs have multiple pps in hevc.the srs can't parse for this.
problem fixed this #3604

Co-authored-by: chundonglinlin chundonglinlin@163.com
Co-authored-by: john hondaxiao@tencent.com

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Jul 29, 2023
@chundonglinlin chundonglinlin changed the title modify:hevc with multiple pps support now Bugfix: HEVC SRT stream supports multiple PPS fields. Aug 2, 2023
@chundonglinlin
Copy link
Member

I'm glad to know that the code modifications I provided worked perfectly for supporting multiple PPS fields. But the gb28181 and utest codes failed the testing because the interface is reused.

@chundonglinlin chundonglinlin linked an issue Aug 3, 2023 that may be closed by this pull request
SRS6.0.55, H265 stream encoded with Shoxi M50 chip, SRT streaming, SRT to RTMP conversion with severe mosaic #3604
Closed
@xengine-qyt
Copy link
Contributor Author

OK ,modified.

@xiaozhihong
Copy link
Collaborator

Is it possible for you to generate a new pull request to add support for multiple PPS in H.264? Thank you.

@xengine-qyt
Copy link
Contributor Author

xengine-qyt commented Aug 7, 2023 •
edited
Loading

yes,of course,just for the h264 of ts?

@xiaozhihong
Copy link
Collaborator

yes,of course,just for the h264 of ts?

Include ts, rtmp and webrtc.

The code below should be refactor.

srs/trunk/src/kernel/srs_kernel_codec.cpp

Lines 2213 to 2224 in 73dd8af

// Support for multiple PPS, then pick the first non-empty one.
for (int i = 0; i < numOfPictureParameterSets; ++i) {
if (!stream->require(2)) {
return srs_error_new(ERROR_HLS_DECODE_ERROR, "decode PPS size");
}
uint16_t pictureParameterSetLength = stream->read_2bytes();
if (!stream->require(pictureParameterSetLength)) {
return srs_error_new(ERROR_HLS_DECODE_ERROR, "decode PPS data");
}
if (pictureParameterSetLength > 0) {
vcodec->pictureParameterSetNALUnit.resize(pictureParameterSetLength);
stream->read_bytes(&vcodec->pictureParameterSetNALUnit[0], pictureParameterSetLength);

@xengine-qyt
Copy link
Contributor Author

why refactor?

@xiaozhihong
Copy link
Collaborator

xiaozhihong commented Aug 8, 2023 •
edited by winlinvip
Loading

why refactor?

Because PR needs to ensure integrity and fully address the issues of multiple PPS, if there are multiple PPS here, the code will be overwritten.

TRANS_BY_GPT3

@chundonglinlin
Copy link
Member

chundonglinlin commented Aug 9, 2023 •
edited
Loading

Two experts, there are also multiple issues with h264's PPS, and there were comments explaining the support for 264 at that time. Should we just confirm and improve this PR or open a new PR to make changes?
@xengine-qyt @xiaozhihong

TRANS_BY_GPT3

@xengine-qyt
Copy link
Contributor Author

why refactor?

Because PR needs to ensure integrity and fully address the issues of multiple PPS, if there are multiple PPS here, the code will be overwritten.

TRANS_BY_GPT3

sorry, I haven't read the code here, so I don't know much about it

@chundonglinlin
Copy link
Member

chundonglinlin commented Aug 10, 2023 •
edited by winlinvip
Loading

why refactor?

Because PR needs to ensure integrity and fully address the issues of multiple PPS, if there are multiple PPS here, the code will be overwritten.
TRANS_BY_GPT3

sorry, I haven't read the code here, so I don't know much about it

Can the h264 pps be modified? The h264 pps is stored using vector<char>, and when numOfPictureParameterSets > 0, it will be overwritten multiple times and only the last pps is recorded. @xengine-qyt '
Please ensure to maintain the markdown structure.

TRANS_BY_GPT4

@xengine-qyt
Copy link
Contributor Author

why refactor?

Because PR needs to ensure integrity and fully address the issues of multiple PPS, if there are multiple PPS here, the code will be overwritten.
TRANS_BY_GPT3

sorry, I haven't read the code here, so I don't know much about it

Can the h264 pps be modified? The h264 pps is stored using vector<char>, and when numOfPictureParameterSets > 0, it will be overwritten multiple times and only the last pps is recorded. @xengine-qyt ' Please ensure to maintain the markdown structure.

TRANS_BY_GPT4

sorry,i have no mulit pps device for h264,I am afraid that the modify is not correct, I recommend you to modify it.

@xiaozhihong
Copy link
Collaborator

xiaozhihong commented Aug 11, 2023 •
edited by winlinvip
Loading

why refactor?

Because PR needs to ensure integrity and fully address the issues of multiple PPS, if there are multiple PPS here, the code will be overwritten.
TRANS_BY_GPT3

sorry, I haven't read the code here, so I don't know much about it

Can the h264 pps be modified? The h264 pps is stored using vector<char>, and when numOfPictureParameterSets > 0, it will be overwritten multiple times and only the last pps is recorded. @xengine-qyt ' Please ensure to maintain the markdown structure.
TRANS_BY_GPT4

sorry,i have no mulit pps device for h264,I am afraid that the modify is not correct, I recommend you to modify it.

Um, okay, thank you very much for the PR, we will handle the rest of the modifications.

@winlinvip winlinvip requested a review from runner365 September 16, 2023 11:05
@winlinvip winlinvip changed the title Bugfix: HEVC SRT stream supports multiple PPS fields. Bugfix: HEVC SRT stream supports multiple PPS fields. v5.0.179 v6.0.76 Sep 18, 2023
@winlinvip winlinvip added the RefinedByAI Refined by AI/GPT. label Sep 18, 2023
@winlinvip winlinvip merged commit 58b9acd into ossrs:develop Sep 18, 2023
17 checks passed
@winlinvip winlinvip changed the title Bugfix: HEVC SRT stream supports multiple PPS fields. v5.0.179 v6.0.76 Bugfix: HEVC SRT stream supports multiple PPS fields. v6.0.76 Sep 18, 2023
winlinvip pushed a commit that referenced this pull request Sep 18, 2023
@xengine-qyt @chundonglinlin
@xiaozhihong @winlinvip
Bugfix: HEVC SRT stream supports multiple PPS fields. v6.0.76 (#3722)
4362df7 
When the srs have multiple pps in hevc.the srs can't parse for this.
problem fixed this #3604

---------

Co-authored-by: chundonglinlin <chundonglinlin@163.com>
Co-authored-by: john <hondaxiao@tencent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiaozhihong xiaozhihong xiaozhihong left review comments

@winlinvip winlinvip winlinvip left review comments

@runner365 runner365 runner365 approved these changes

@chundonglinlin chundonglinlin chundonglinlin approved these changes

Assignees
No one assigned
Labels
EnglishNative This issue is conveyed exclusively in English. RefinedByAI Refined by AI/GPT.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SRS6.0.55, H265 stream encoded with Shoxi M50 chip, SRT streaming, SRT to RTMP conversion with severe mosaic
5 participants
@xengine-qyt @chundonglinlin @xiaozhihong @winlinvip @runner365