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

HTTP-FLV: Notify connection to expire when unpublishing. v6.0.152 v7.0.11 #4164

Merged
merged 4 commits into from
Aug 31, 2024
Merged

HTTP-FLV: Notify connection to expire when unpublishing. v6.0.152 v7.0.11 #4164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f6736af
HTTP-FLV: Notify connection to expire.
winlinvip Aug 31, 2024
8dc90e0
Fix warning.
winlinvip Aug 31, 2024
5196dcf
Refine code.
winlinvip Aug 31, 2024
db8e747
Update release to v6.0.152 v7.0.11
winlinvip Aug 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions trunk/doc/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ The changelog for SRS.
<a name="v7-changes"></a>

## SRS 7.0 Changelog
* v7.0, 2024-08-31, Merge [#4164](https://github.com/ossrs/srs/pull/4164): HTTP-FLV: Notify connection to expire when unpublishing. v7.0.11 (#4164)
* v7.0, 2024-08-24, Merge [#4157](https://github.com/ossrs/srs/pull/4157): Fix crash when quiting. v7.0.10 (#4157)
* v7.0, 2024-08-24, Merge [#4156](https://github.com/ossrs/srs/pull/4156): Build: Fix srs_mp4_parser compiling error. v7.0.9 (#4156)
* v7.0, 2024-08-22, Merge [#4154](https://github.com/ossrs/srs/pull/4154): ASAN: Disable memory leak detection by default. v7.0.8 (#4154)
Expand All @@ -22,6 +23,7 @@ The changelog for SRS.
<a name="v6-changes"></a>

## SRS 6.0 Changelog
* v6.0, 2024-08-31, Merge [#4164](https://github.com/ossrs/srs/pull/4164): HTTP-FLV: Notify connection to expire when unpublishing. v6.0.152 (#4164)
* v6.0, 2024-08-24, Merge [#4157](https://github.com/ossrs/srs/pull/4157): Fix crash when quiting. v6.0.151 (#4157)
* v6.0, 2024-08-24, Merge [#4156](https://github.com/ossrs/srs/pull/4156): Build: Fix srs_mp4_parser compiling error. v6.0.150 (#4156)
* v6.0, 2024-08-21, Merge [#4150](https://github.com/ossrs/srs/pull/4150): API: Support new HTTP API for VALGRIND. v6.0.149 (#4150)
Expand Down
2 changes: 0 additions & 2 deletions trunk/src/app/srs_app_http_api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,8 +1206,6 @@ SrsGoApiSignal::~SrsGoApiSignal()

srs_error_t SrsGoApiSignal::serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r)
{
srs_error_t err = srs_success;

std::string signal = r->query_get("signo");
srs_trace("query signo=%s", signal.c_str());

Expand Down
30 changes: 25 additions & 5 deletions trunk/src/app/srs_app_http_stream.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,13 +583,15 @@ SrsLiveStream::SrsLiveStream(SrsRequest* r, SrsBufferCache* c)
cache = c;
req = r->copy()->as_http();
security_ = new SrsSecurity();
alive_viewers_ = 0;
}

SrsLiveStream::~SrsLiveStream()
{
srs_freep(req);
srs_freep(security_);

// The live stream should never be destroyed when it's serving any viewers.
srs_assert(viewers_.empty());
}

srs_error_t SrsLiveStream::update_auth(SrsRequest* r)
Expand Down Expand Up @@ -634,18 +636,35 @@ srs_error_t SrsLiveStream::serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage
return srs_error_wrap(err, "http hook");
}

alive_viewers_++;
// Add the viewer to the viewers list.
viewers_.push_back(hc);

// Serve the viewer connection.
err = do_serve_http(w, r);
alive_viewers_--;


// Remove viewer from the viewers list.
vector<ISrsExpire*>::iterator it = std::find(viewers_.begin(), viewers_.end(), hc);
srs_assert (it != viewers_.end());
viewers_.erase(it);
Copy link
Contributor

Choose a reason for hiding this comment

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

viewers_, as vector container , has bad efficiency in erase, and also find for unsorted data.
For one SRS instance, it is reasonable to support 1K flv http connection.
So it's 1K unsorted vector search and delete frequently. (Not so bad, also not so good).
If the vector size is 10K, it will be serious problem.

In those reason, consider map or unordered_map as the container. the SrsHttpConn->get_id as the key, make sure the key is unique for each SrsHttpConn.

Copy link
Member Author

@winlinvip winlinvip Aug 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No performance issues even for 10,000 viewers, because this only executes once when a viewer closes the connection.

If the vector size is 10,000, it will be a serious problem.

I have written a test program to verify it; it is definitely not that slow. We should never fix problems that do not exist.

Copy link
Member Author

@winlinvip winlinvip Aug 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Try bellow benchmark, to randomly find a elem in 100k vector:

#include <iostream>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <chrono>
using namespace std;
using namespace chrono;

int main() {
    int max_elements = 100 * 1000;

    vector<uint64_t> vec;
    for (uint64_t i = 0; i < max_elements; ++i) {
        vec.push_back(i);
    }

    srand(static_cast<unsigned>(time(0)));
    for (int i = 0; i < 10; i++) {
        size_t random_index = rand() % max_elements;
        uint64_t value = vec[random_index];

        auto start = high_resolution_clock::now();
        vector<uint64_t>::iterator it = std::find(vec.begin(), vec.end(), value);
        auto end = high_resolution_clock::now();

        auto duration = duration_cast<nanoseconds>(end - start).count() / 1000.0;
        cout << "Element at index " << random_index << " is " << value << " find " << *it << endl;
        cout << "Time taken to find the element: " << duration << " microseconds" << endl << endl;
    }

    return 0;
}

Result:

Element at index 0 is 0 find 0
Time taken to find the element: 0.166 microseconds

Element at index 30000 is 30000 find 30000
Time taken to find the element: 77.25 microseconds

Element at index 20000 is 20000 find 20000
Time taken to find the element: 50.5 microseconds

Element at index 30000 is 30000 find 30000
Time taken to find the element: 75.125 microseconds

Element at index 70000 is 70000 find 70000
Time taken to find the element: 172.541 microseconds

Element at index 30000 is 30000 find 30000
Time taken to find the element: 74.042 microseconds

Element at index 90000 is 90000 find 90000
Time taken to find the element: 223.125 microseconds

Element at index 80000 is 80000 find 80000
Time taken to find the element: 199.542 microseconds

Element at index 20000 is 20000 find 20000
Time taken to find the element: 51.875 microseconds

Element at index 50000 is 50000 find 50000
Time taken to find the element: 123.084 microseconds

It only increase 0.1ms when viewer closing the connection, note that in 100k vector, not 10k.

Copy link
Contributor

Choose a reason for hiding this comment

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

one viewer close the http connection, it exec one time, but if 10K viewers close http flv connections, this code will run 10K times. The rarely case is the 10K viewers close the connection at same time. OK, I accept this.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the benchmark, The real disaster of the vector is the erase. I would avoid doing this as far as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even with 10,000 viewers, each increase is only 0.01ms. When will 10,000 viewers close the connection simultaneously? We should never fix problems that do not exist. You will be completely occupied if you try to solve every imaginary problem that does not exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, accept it.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the benchmark, The real disaster of the vector is the erase. I would avoid doing this as far as possible.

Try this:

#include <iostream>
#include <vector>
#include <cstdlib>
#include <ctime>
#include <chrono>
using namespace std;
using namespace chrono;

int main() {
    int max_elements = 10 * 1000;

    vector<uint64_t> vec;
    for (uint64_t i = 0; i < max_elements; ++i) {
        vec.push_back(i);
    }

    srand(static_cast<unsigned>(time(0)));
    for (int i = 0; i < 10; i++) {
        size_t random_index = rand() % vec.size();
        uint64_t value = vec[random_index];

        auto start = high_resolution_clock::now();
        vector<uint64_t>::iterator it = std::find(vec.begin(), vec.end(), value);
        vec.erase(it);
        auto end = high_resolution_clock::now();

        auto duration = duration_cast<nanoseconds>(end - start).count() / 1000.0;
        cout << "Element at index " << random_index << " is " << value << " find " << *it << endl;
        cout << "Time taken to find the element: " << duration << " microseconds" << endl << endl;
    }

    return 0;
}

The result:

Element at index 8555 is 8555 find 8556
Time taken to find the element: 21 microseconds

Element at index 8770 is 8771 find 8772
Time taken to find the element: 25.25 microseconds

Element at index 7152 is 7152 find 7153
Time taken to find the element: 18.25 microseconds

Element at index 2981 is 2981 find 2982
Time taken to find the element: 8.5 microseconds

Element at index 2102 is 2102 find 2103
Time taken to find the element: 6.292 microseconds

Element at index 5273 is 5275 find 5276
Time taken to find the element: 13.709 microseconds

Element at index 8957 is 8963 find 8964
Time taken to find the element: 22.417 microseconds

Element at index 1154 is 1154 find 1155
Time taken to find the element: 4 microseconds

Element at index 9198 is 9206 find 9207
Time taken to find the element: 23.292 microseconds

Element at index 7305 is 7310 find 7311
Time taken to find the element: 21.625 microseconds

For 10,000 viewers, each increase is 0.02ms. I think the key point is to run some benchmarks and gather data when you are concerned about performance issues.


// Do hook after serving.
http_hooks_on_stop(r);

return err;
}

bool SrsLiveStream::alive()
{
return alive_viewers_ > 0;
return !viewers_.empty();
}

void SrsLiveStream::expire()
{
vector<ISrsExpire*>::iterator it;
for (it = viewers_.begin(); it != viewers_.end(); ++it) {
ISrsExpire* conn = *it;
conn->expire();
}
}

srs_error_t SrsLiveStream::do_serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r)
Expand Down Expand Up @@ -1075,6 +1094,7 @@ void SrsHttpStreamServer::http_unmount(SrsRequest* r)

// Notify cache and stream to stop.
if (stream->entry) stream->entry->enabled = false;
stream->expire();
cache->stop();

// Wait for cache and stream to stop.
Copy link
Contributor

Choose a reason for hiding this comment

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

stream->expire() will disconnect the SrsHTTPConn soon, so no long to wait 120 seconds below. Maybe 10 seconds is OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

No essential difference. I want keep it.

Expand Down
9 changes: 7 additions & 2 deletions trunk/src/app/srs_app_http_stream.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include <srs_app_security.hpp>
#include <srs_app_http_conn.hpp>

#include <vector>

class SrsAacTransmuxer;
class SrsMp3Transmuxer;
class SrsFlvTransmuxer;
Expand Down Expand Up @@ -176,7 +178,7 @@ class SrsBufferWriter : public SrsFileWriter

// HTTP Live Streaming, to transmux RTMP to HTTP FLV or other format.
// TODO: FIXME: Rename to SrsHttpLive
class SrsLiveStream : public ISrsHttpHandler
class SrsLiveStream : public ISrsHttpHandler, public ISrsExpire
{
private:
SrsRequest* req;
Expand All @@ -185,14 +187,17 @@ class SrsLiveStream : public ISrsHttpHandler
// For multiple viewers, which means there will more than one alive viewers for a live stream, so we must
// use an int value to represent if there is any viewer is alive. We should never do cleanup unless all
// viewers closed the connection.
int alive_viewers_;
std::vector<ISrsExpire*> viewers_;
public:
SrsLiveStream(SrsRequest* r, SrsBufferCache* c);
virtual ~SrsLiveStream();
virtual srs_error_t update_auth(SrsRequest* r);
public:
virtual srs_error_t serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r);
virtual bool alive();
// Interface ISrsExpire
public:
virtual void expire();
private:
virtual srs_error_t do_serve_http(ISrsHttpResponseWriter* w, ISrsHttpMessage* r);
virtual srs_error_t http_hooks_on_play(ISrsHttpMessage* r);
Expand Down
2 changes: 1 addition & 1 deletion trunk/src/core/srs_core_version6.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@

#define VERSION_MAJOR 6
#define VERSION_MINOR 0
#define VERSION_REVISION 151
#define VERSION_REVISION 152

#endif
2 changes: 1 addition & 1 deletion trunk/src/core/srs_core_version7.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@

#define VERSION_MAJOR 7
#define VERSION_MINOR 0
#define VERSION_REVISION 10
#define VERSION_REVISION 11

#endif
Loading