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

Fix crash issue at srs_app_encoder #908

Closed
wants to merge 1 commit into from
Closed

Fix crash issue at srs_app_encoder #908

wants to merge 1 commit into from

Conversation

HungMingWu
Copy link
Contributor

Root Cause:
On SrsEncoder::on_publish, if ret is equal to ERROR_ENCODER_LOOP and ffmpegs is empty.
It wouldn't create a trd object.
On SrsEncoder::on_unpublish:
Since the trd is NULL, it would crash.

@winlinvip
Copy link
Member

winlinvip commented Jun 3, 2017

I think it's better to create a dummy thread for classes like the SrsEncoder which need to create thread dynamically, for example:

SrsEncoder() {
    trd = new SrsDummyCoroutine();
}
~SrsEncoder() {
    srs_freep(trd);
}
int publish() {
    srs_freep(trd);
    trd = new SrsCoroutine();
    trd->start();
}
void unpublish() {
    srs_freep(trd);
}

class SrsDummyCoroutine {
    SrsDummyCoroutine() {
    }
    int start() {
        return ErrorEmptyObject;
    }
    void stop() {
    }
}

I recall it's a common design pattern named Empty Object. An empty object is always better than a NULL pointer, because the object has some behaviors, like the trd->stop(). In golang, all objects are initialized with zero values, like this pattern.

@winlinvip
Copy link
Member

winlinvip commented Jun 3, 2017

Please fix this issue at all places that use trd = NULL, we should use the dummy thread instead, which simply return failed when start and ignore when stop.

@HungMingWu
Copy link
Contributor Author

HungMingWu commented Jun 3, 2017 via email

@winlinvip
Copy link
Member

winlinvip commented Jun 4, 2017

That's sure the constructor is called multiple times in on_publish, but I think it's fine because it wouldn't hurt the performance(these functions is rarely called). Please use the empty object to solve this problem, thanks~

@HungMingWu
Copy link
Contributor Author

I have update the commit.

@winlinvip
Copy link
Member

Hi, thanks for your work, but I fixed it in 9ca3697

@winlinvip winlinvip closed this Jun 4, 2017
@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
TransByAI Translated by AI/GPT.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants