-
Notifications
You must be signed in to change notification settings - Fork 861
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
Ensure stdout silence in console pipe out mode plus a few minor fixups #275
Conversation
rndi
commented
Feb 13, 2018
- Ensure stdout silence in console pipe out mode
- Ignore non-positive number timeouts
- Fix target validation typo
- Adjust help wording
apps/srt-live-transmit.cpp
Outdated
@@ -300,6 +300,13 @@ int main( int argc, char** argv ) | |||
return 1; | |||
} | |||
|
|||
if (params[1].rfind("file://con", 0) == 0) | |||
{ | |||
// ensure we are quiet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's rather a wrong approach - if a user explicitly requested verbose mode and output to stdout, issue an error and exit with failure. The user should be informed that this combination is impossible, not that it "silently doesn't work".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change will make the application "silently work" when destination is stdout. I think this is way better approach than needlessly barking at the user since user's intention is clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Users will complain "why the -v option doesn't work?". Moreover, as suggested below, you can make an additional option like -v2
that will do verbose logs to stderr.
apps/transmitmedia.cpp
Outdated
@@ -949,10 +949,10 @@ class UdpCommon | |||
{ | |||
int ttl = stoi(attr.at("ttl")); | |||
int res = setsockopt(m_sock, IPPROTO_IP, IP_TTL, (const char*)&ttl, sizeof ttl); | |||
if (res == -1) | |||
if (transmit_verbose && res == -1) | |||
cout << "WARNING: failed to set 'ttl' (IP_TTL) to " << ttl << endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This transmit_verbose
is now renamed to Verbose::on
. And it's easier to simply change cout
into Verb()
and remove << endl
.
Shouldn't verbose (or any in that matter) console log output go to stderr
instead? Ffmpeg does it this way so that logs don't interfere with
input/output pipes.
2018-02-13 9:43 GMT+01:00 Sektor van Skijlen <notifications@github.com>:
… ***@***.**** requested changes on this pull request.
------------------------------
In apps/srt-live-transmit.cpp
<#275 (comment)>:
> @@ -300,6 +300,13 @@ int main( int argc, char** argv )
return 1;
}
+ if (params[1].rfind("file://con", 0) == 0)
+ {
+ // ensure we are quiet
That's rather a wrong approach - if a user explicitly requested verbose
mode and output to stdout, issue an error and exit with failure. The user
should be informed that this combination is impossible, not that it
"silently doesn't work".
------------------------------
In apps/transmitmedia.cpp
<#275 (comment)>:
> @@ -949,10 +949,10 @@ class UdpCommon
{
int ttl = stoi(attr.at("ttl"));
int res = setsockopt(m_sock, IPPROTO_IP, IP_TTL, (const char*)&ttl, sizeof ttl);
- if (res == -1)
+ if (transmit_verbose && res == -1)
cout << "WARNING: failed to set 'ttl' (IP_TTL) to " << ttl << endl;
This transmit_verbose is now renamed to Verbose::on. And it's easier to
simply change cout into Verb() and remove << endl.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#275 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB049mGmQccDx7LfxP9wX95lzGMW7wMCks5tUUsugaJpZM4SDNcK>
.
|
The trouble is that we do use stderr in this app for some other kind of messages, such that cannot be silenced, which inform about some serious problems. Actually the verbose logging was predicted only for testing purposes, including user testing, as this displays the size of every packet that was passed between two media, so it's usually connected with storing the stream into a file. I think we might at least use an option to turn on verbose mode, but redirect it to stderr. @rndi I think there could be something like |
* dev: (21 commits) Fix SRT file transmit app to work in non-blocking mode (Haivision#334) Ensure stdout silence in console pipe out mode plus a few minor fixups (Haivision#275) Replaced hardcoded installdirs with GNUInstallDirs. Fixed some status messages. (Haivision#323) Breaking connection when recv buffer inflation caused sequence discrepancy (Haivision#300) Fixed problems with encryption decision and status report on encryption failure. (Haivision#318) Fixed invalid symbol names in doc (Haivision#311) Dev add version to winpackages (Haivision#328) Android build for dev (Haivision#326) Change License to MPLv2.0 (Haivision#327) Used constants for input rate. Fixed after-start rate sampling period to 1s (Haivision#315) Made SockaddrToString use only numeric string by default (Haivision#312) Ported change in Haivision#307 PR to dev Fixed SockaddrToString to format as 4dotIP if unknown (Haivision#299) Build with debug information for lldb on iOS platform (Haivision#302) Fix for sudden stop sending data on macOS/iOS (Haivision#303) Fix broken build when testing apps enabled (Haivision#296) Removed all code introduced for CBR (Haivision#293) Added API to get instantaneous stats instead of moving averages for a… (Haivision#288) Added toolchain file and build instruction for iOS (Haivision#286) Fix windows build (Haivision#290) ...