Skip to content

Transcode: Bugfix: Fix loop transcoding with host. #3516. v6.0.168 v7.0.41 #4325

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

Merged
merged 7 commits into from
Jun 4, 2025

Conversation

duiniuluantanqin
Copy link
Member

@duiniuluantanqin duiniuluantanqin commented Apr 15, 2025

What issue has been resolved?

for issue: #3516 #4055 #3618

What is the root cause of the problem?

The issue arises from a mismatch between the input and output formats within the SrsEncoder::initialize_ffmpeg function.

For example:
Input: rtmp://127.0.0.1:1935/live?vhost=__defaultVhost__/livestream_ff
Output: rtmp://127.0.0.1:1935/live/livestream_ff?vhost=__defaultVhost__

This may result in the failure of the code segment responsible for determining whether to loop.

What is the approach to solving this issue?

It simply involves modifying the order of stream and vhost.

How was the issue introduced?

The commit introducing this bug is: 7d47017
The order of parameters in the configuration file has been modified to address the ingest issue.

Outstanding issues

Please note that this PR does not entirely resolve the issue; for example, modifying the output format in configuration still results in exceptions. To comprehensively address this problem, extensive code modifications would be required.

However, strictly adhering to the configuration file format can effectively prevent this issue.


Co-authored-by: Jacob Su [email protected]
Co-authored-by: john [email protected]

@winlinvip winlinvip added the EnglishNative This issue is conveyed exclusively in English. label Apr 15, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

trunk/src/app/srs_app_encoder.cpp:243

  • Passing req->vhost twice may be unintentional. Please verify the parameter order for srs_generate_rtmp_url to ensure that all components of the URL are correct.
std::string input = srs_generate_rtmp_url(SRS_CONSTS_LOCALHOST, req->port, req->vhost, req->vhost, req->app, req->stream, req->param);

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses bug #3516 by correcting the RTMP URL formatting in the SrsEncoder::initialize_ffmpeg function.

  • Replaces manual string concatenation with srs_generate_rtmp_url.
  • Modifies parameter ordering to address the inversion of 'stream' and 'vhost'.

input += "/";
input += req->stream;

std::string input = srs_generate_rtmp_url(SRS_CONSTS_LOCALHOST, req->port, req->vhost, req->vhost, req->app, req->stream, req->param);
Copy link
Preview

Copilot AI Apr 15, 2025

Choose a reason for hiding this comment

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

The function call to srs_generate_rtmp_url explicitly passes 'req->vhost' twice; please verify that the parameter order is correct to ensure the URL is generated with 'stream' and 'vhost' in the intended positions.

Suggested change
std::string input = srs_generate_rtmp_url(SRS_CONSTS_LOCALHOST, req->port, req->vhost, req->vhost, req->app, req->stream, req->param);
std::string input = srs_generate_rtmp_url(SRS_CONSTS_LOCALHOST, req->port, req->vhost, req->stream, req->app, req->stream, req->param);

Copilot uses AI. Check for mistakes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this patch really fix the bug?
The bug said the transcode stream can't output to same srs instance with same vhost.
output rtmp://127.0.0.1:[port]/[app]/[stream]_[engine]?vhost=[vhost];
It seems this PR didn't improve anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

The root cause of the issue lies in the order of the vhosts, which leads this segment of code to fail; therefore, merely rearranging the order resolves the problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've revised the code, which should render it more comprehensible.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reorders the URL parameter placement in the encoder initialization to resolve bugs related to parameter mismatches.

  • Reorders the placement of the "vhost" parameter from before the stream to after the stream.
  • Aims to fix looping issues by aligning the input and output URL formats.
Comments suppressed due to low confidence (1)

trunk/src/app/srs_app_encoder.cpp:252

  • The reordering of the URL parameters appears to address the reported issue, but please verify that the new sequence handles edge cases such as streams with existing query parameters; consider adding unit tests to ensure robustness.
input += "?vhost=";

@duiniuluantanqin duiniuluantanqin changed the title fix bug: #3516 fix bug: loop transcoding #3516 Apr 28, 2025
@winlinvip winlinvip changed the title fix bug: loop transcoding #3516 fix bug: loop transcoding #3516. v6.0.168 v7.0.41 Jun 4, 2025
@winlinvip winlinvip added the RefinedByAI Refined by AI/GPT. label Jun 4, 2025
@winlinvip winlinvip changed the title fix bug: loop transcoding #3516. v6.0.168 v7.0.41 Transcode: Bugfix: Fix loop transcoding with host #3516. v6.0.168 v7.0.41 Jun 4, 2025
@winlinvip winlinvip changed the title Transcode: Bugfix: Fix loop transcoding with host #3516. v6.0.168 v7.0.41 Transcode: Bugfix: Fix loop transcoding. #3516. v6.0.168 v7.0.41 Jun 4, 2025
@winlinvip winlinvip changed the title Transcode: Bugfix: Fix loop transcoding. #3516. v6.0.168 v7.0.41 Transcode: Bugfix: Fix loop transcoding with host. #3516. v6.0.168 v7.0.41 Jun 4, 2025
@winlinvip winlinvip merged commit 133866a into ossrs:develop Jun 4, 2025
17 checks passed
winlinvip added a commit that referenced this pull request Jun 4, 2025
…4325)

for issue: #3516
#4055
#3618

The issue arises from a mismatch between the `input` and `output`
formats within the
[`SrsEncoder::initialize_ffmpeg`](https://github.com/ossrs/srs/pull/4325/files#diff-a3dd7c498fc26d36def2e8c2c3b7edfe1bf78f0620b1a838aefa70ba119cad03L241-L254)
function.

For example:
Input: `rtmp://127.0.0.1:1935/live?vhost=__defaultVhost__/livestream_ff`
Output:
`rtmp://127.0.0.1:1935/live/livestream_ff?vhost=__defaultVhost__`

This may result in the failure of the [code
segment](https://github.com/ossrs/srs/pull/4325/files#diff-a3dd7c498fc26d36def2e8c2c3b7edfe1bf78f0620b1a838aefa70ba119cad03L292-L298)
responsible for determining whether to loop.

It simply involves modifying the order of `stream` and `vhost`.

The commit introducing this bug is:
7d47017
The order of [parameters in the configuration
file](7d47017#diff-428de168925d659dae72bb49273c3b048ed2800906c6848560badae854250126L26-R26)
has been modified to address the `ingest` issue.

Please note that this PR does not entirely resolve the issue; for
example, modifying the `output` format in configuration still results in
exceptions. To comprehensively address this problem, extensive code
modifications would be required.

However, strictly adhering to the configuration file format can
effectively prevent this issue.

---------

Co-authored-by: Jacob Su <[email protected]>
Co-authored-by: john <[email protected]>
Co-authored-by: winlin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
EnglishNative This issue is conveyed exclusively in English. RefinedByAI Refined by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants