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

Add support for SRTO_STREAMID #34

Merged
merged 5 commits into from
Jan 21, 2022
Merged

Add support for SRTO_STREAMID #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ef81c01
Update node-srt.cc
Morpheus235 Jan 19, 2022
ebc16b9
Add SRTO_STREAM to SOCKET_OPTIONS Enum
Morpheus235 Jan 19, 2022
d061036
fix typo
Morpheus235 Jan 19, 2022
64076e3
Add Test ffor SRTO_STREAMID
Morpheus235 Jan 19, 2022
bb9665b
finished SRTO_STREAMID implementation
Systempiloten Jan 21, 2022
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
13 changes: 13 additions & 0 deletions spec/async_srt_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,19 @@ describe("Async SRT API with callbacks", () => {
});
});

it("can set SRT sockopt SRTO_STREAMID", done => {
const asyncSrt = new AsyncSRT();
asyncSrt.createSocket(false, (socket) => {
asyncSrt.setSockOpt(socket, SRT.SRTO_STREAMID, "STREAMID", (result) => {
expect(result).not.toEqual(SRT.ERROR);
asyncSrt.getSockOpt(socket, SRT.SRTO_STREAMID, (value) => {
expect(value).toEqual("STREAMID");
done();
});
});
});
});

it("can set SRT socket in non-blocking mode", done => {
const asyncSrt = new AsyncSRT();
asyncSrt.createSocket(false, (socket) => {
Expand Down
15 changes: 13 additions & 2 deletions spec/srt_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,17 @@ describe("SRT library", () => {
expect(value).toEqual(1052);
});

it("can set SRT sockopt SRTO_STREAMID", () => {
const srt = new SRT();
const socket = srt.createSocket();
const result = srt.setSockOpt(socket, SRT.SRTO_STREAMID, "STREAMID");

expect(result).not.toEqual(SRT.ERROR);
const value = srt.getSockOpt(socket, SRT.SRTO_STREAMID);

expect(value).toEqual("STREAMID");
});

it("can set SRT socket in non-blocking mode", () => {
const srt = new SRT();
const socket = srt.createSocket();
Expand All @@ -59,12 +70,12 @@ describe("SRT library", () => {
const epid = srt.epollCreate();
srt.epollAddUsock(epid, socket, SRT.EPOLL_IN | SRT.EPOLL_ERR);
const events = srt.epollUWait(epid, 500);

expect(events.length).toEqual(0);
});

it("exposes socket options", () => {
expect(SRT.SRTO_UDP_SNDBUF).toEqual(8);
expect(SRT.SRTO_RCVLATENCY).toEqual(43);
});
});
});
11 changes: 11 additions & 0 deletions src/node-srt.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,15 @@ Napi::Value NodeSRT::SetSockOpt(const Napi::CallbackInfo& info) {
Napi::Error::New(env, srt_getlasterror_str()).ThrowAsJavaScriptException();
return Napi::Number::New(env, SRT_ERROR);
}
} else if (info[2].IsString()) {
Napi::String value = info[2].As<Napi::String>();
int32_t optName = option;
const char * optValue = std::string(value).c_str();
Copy link
Contributor

Choose a reason for hiding this comment

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

Clang compiler spits out a warning here for understandable reasons.

that std::string objects may be kicked off the stack just after c_str returns.

meaning that the mem at the pointer is freed then, and potentially the value passed not ensured to be what you expect here at all (even if this may work, you actually pass a pointer to already deallocated mem here every time).

result = srt_setsockflag(socketValue, (SRT_SOCKOPT)optName, optValue, sizeof(string));
Copy link
Contributor

@tchakabam tchakabam Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

sizeof(string) is an relatively arbitrary and likely platform C++ runtime lib dependent allocation of the std::string container size, as in how much stack/heap gets allocated for that object as in per instance :D that is a compile-time constant also obviously. It is not related the string length, because that class is just a container for the underlying C-string data buffer.

The srt_setsockflag expects the size of the data behind the pointer passed. that is constant for bools and ints ie numbers. In case of a string, the length of the string is expected.

Copy link
Contributor

@tchakabam tchakabam Apr 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Just adding here also, this does "somehow" work as long as your string data passed is smaller than whatever sizeof(string) gives you. But that shouldnt be the idea here ;)

probably somewhat contributing to it having worked at all is that C-string is null-terminated, so even if libSRT copies this into a too large buffer along with some garbage, it reads the string properly ... but still this could crash as potentially letting srt_setsockflag read/copy from random place in memory itfp -- just in case this ever crashed for you; that is probably why.

if (result == SRT_ERROR) {
Napi::Error::New(env, srt_getlasterror_str()).ThrowAsJavaScriptException();
return Napi::Number::New(env, SRT_ERROR);
}
}
return Napi::Number::New(env, result);
}
Expand Down Expand Up @@ -344,10 +353,12 @@ Napi::Value NodeSRT::GetSockOpt(const Napi::CallbackInfo& info) {
}
case SRTO_PACKETFILTER:
case SRTO_PASSPHRASE:
case SRTO_STREAMID:
{
char optValue[512];
int optSize = sizeof(optValue);
result = srt_getsockflag(socketValue, (SRT_SOCKOPT)optName, (void *)&optValue, &optSize);

returnVal = Napi::Value::From(env, std::string(optValue));
break;
}
Expand Down