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

GB28181: Fix #2570, remove useless audio track info in PMT when no audio #2738

Closed

Conversation

qichaoshen82
Copy link

Fix #2570, remove useless audio track info in PMT when no audio

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2021

Codecov Report

Merging #2738 (6558010) into feature/gb28181 (f6c445b) will increase coverage by 0.01%.
The diff coverage is 59.72%.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           feature/gb28181    #2738      +/-   ##
===================================================
+ Coverage            59.81%   59.83%   +0.01%     
===================================================
  Files                  121      121              
  Lines                51195    51246      +51     
===================================================
+ Hits                 30622    30661      +39     
- Misses               20573    20585      +12     

| Impacted Files | Coverage Δ | |'

Translated to English while maintaining the markdown structure:

'| Impacted Files | Coverage Δ | |
|---|---|---|
| trunk/src/app/srs_app_config.cpp | 62.75% <ø> (ø) | |
| trunk/src/app/srs_app_hls.cpp | 0.00% <0.00%> (ø) | |'

Translated to English while maintaining the markdown structure:

'| trunk/src/app/srs_app_config.cpp | 62.75% <ø> (ø) | |
| trunk/src/app/srs_app_hls.cpp | 0.00% <0.00%> (ø) | |
| trunk/src/kernel/srs_kernel_ts.cpp | 82.08% <68.25%> (-0.82%) | ⬇️ |


Continue to review full report at Codecov.

Legend - Click here to learn more
| Δ = absolute <relative> (impact), ø = not affected, ? = missing data'

Translated to English while maintaining the markdown structure:

'| Δ = absolute <relative> (impact), ø = not affected, ? = missing data

Powered by Codecov. Last update f6c445b...6558010. Read the comment docs.

TRANS_BY_GPT3

@winlinvip
Copy link
Member

Thanks for your PR, assign to @duiniuluantanqin 😄

@winlinvip winlinvip added the GB28181 For GB28181. label Nov 16, 2021
@winlinvip winlinvip changed the title Fix #2570, remove useless audio track info in PMT when no audio GB28181: Fix #2570, remove useless audio track info in PMT when no audio Nov 16, 2021
@qichaoshen82
Copy link
Author

qichaoshen82 commented Nov 18, 2021

Delete codec id from the configuration file and add codec settings in HLS.
@duiniuluantanqin @winlinvip

TRANS_BY_GPT3

@duiniuluantanqin
Copy link
Member

duiniuluantanqin commented Nov 22, 2021

@winlinvip I suggest merging this PR into the develop branch. It's not just a problem with GB, but a common issue. Even during live streaming, the problem can be reproduced without enabling audio.

TRANS_BY_GPT3

@@ -2760,7 +2760,7 @@ srs_error_t SrsConfig::check_normal_config()
for (int j = 0; j < (int)conf->directives.size(); j++) {
string m = conf->at(j)->name;
if (m != "enabled" && m != "hls_entry_prefix" && m != "hls_path" && m != "hls_fragment" && m != "hls_window" && m != "hls_on_error"
&& m != "hls_storage" && m != "hls_mount" && m != "hls_td_ratio" && m != "hls_aof_ratio" && m != "hls_acodec" && m != "hls_vcodec"
&& m != "hls_storage" && m != "hls_mount" && m != "hls_td_ratio" && m != "hls_aof_ratio"
Copy link
Member

@winlinvip winlinvip Nov 23, 2021

Choose a reason for hiding this comment

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

If these two options have been configured before, it will not run directly, resulting in user startup failure.

This is an incompatible change, and users cannot use it. Refer to srs_config_transform_vhost to delete these two configuration items (and print a warning), so that they can be safely removed.

TRANS_BY_GPT3

@@ -3522,13 +3522,11 @@ VOID TEST(ConfigMainTest, CheckVhostConfig5)

if (true) {
MockSrsConfig conf;
HELPER_ASSERT_SUCCESS(conf.parse(_MIN_OK_CONF "vhost ossrs.net{hls{hls_td_ratio 2.1;hls_aof_ratio 3.1;hls_window 10;hls_on_error xxx;hls_acodec xxx2;hls_vcodec xxx3;hls_nb_notify 5;hls_dts_directly off;hls_cleanup off;hls_dispose 10;hls_wait_keyframe off;}}"));
HELPER_ASSERT_SUCCESS(conf.parse(_MIN_OK_CONF "vhost ossrs.net{hls{hls_td_ratio 2.1;hls_aof_ratio 3.1;hls_window 10;hls_on_error xxx;hls_nb_notify 5;hls_dts_directly off;hls_cleanup off;hls_dispose 10;hls_wait_keyframe off;}}"));
Copy link
Member

@winlinvip winlinvip Nov 23, 2021

Choose a reason for hiding this comment

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

This 'utest' cannot remove this configuration item, as it indicates that some users may have configured it this way, and it should not cause them to fail.

On the contrary, we should make the configuration compatible and allow this 'utest' to pass.

TRANS_BY_GPT3

for (idx=0; idx<size; idx++) {
msg = ts_msg_cache_for_verify_codec.at(idx);
if(msg) {
context->encode(writer, msg, vcodec, acodec);
Copy link
Member

@winlinvip winlinvip Nov 23, 2021

Choose a reason for hiding this comment

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

srs_error_t SrsTsContext::encode(ISrsStreamWriter* writer, SrsTsMessage* msg, SrsVideoCodecId vc, SrsAudioCodecId ac)

This function will return an error, and the error is lost here. When an error occurs, we have no idea what happened at all.

TRANS_BY_GPT3

trunk/src/app/srs_app_config.cpp Outdated Show resolved Hide resolved
trunk/src/app/srs_app_hls.cpp Show resolved Hide resolved
Copy link

@TanessaW TanessaW left a comment

Choose a reason for hiding this comment

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

@TanessaW

This comment was marked as spam.

@winlinvip
Copy link
Member

Migrate to ossrs/srs-gb28181#2

@winlinvip winlinvip closed this Jan 5, 2022
@winlinvip winlinvip added the TransByAI Translated by AI/GPT. label Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GB28181 For GB28181. TransByAI Translated by AI/GPT.
Projects
None yet
9 participants