[FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Rodger Combs]

---
 ffmpeg.c | 4 +---
 ffmpeg_opt.c | 3 +++
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index d858407..111d844 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
 void term_init(void)
 {
 #if HAVE_TERMIOS_H
-if(!run_as_daemon){
+if (!run_as_daemon && stdin_interaction) {
 struct termios tty;
 if (tcgetattr (0, ) == 0) {
 oldtty = tty;
@@ -4328,8 +4328,6 @@ int main(int argc, char **argv)
 
 show_banner(argc, argv, options);
 
-term_init();
-
 /* parse options and open all input/output files */
 ret = ffmpeg_parse_options(argc, argv);
 if (ret < 0)
diff --git a/ffmpeg_opt.c b/ffmpeg_opt.c
index 2ea09cf..4d3d4c4 100644
--- a/ffmpeg_opt.c
+++ b/ffmpeg_opt.c
@@ -3102,6 +3102,9 @@ int ffmpeg_parse_options(int argc, char **argv)
 goto fail;
 }
 
+/* configure terminal and setup signal handlers */
+term_init();
+
 /* open input files */
 ret = open_files([GROUP_INFILE], "input", open_input_file);
 if (ret < 0) {
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Nicolas George]

Le quintidi 25 fructidor, an CCXXIV, Rodger Combs a écrit :
> ---
>  ffmpeg.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index d858407..08a7a3d 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
>  void term_init(void)
>  {
>  #if HAVE_TERMIOS_H
> -if(!run_as_daemon){
> +if (!run_as_daemon && stdin_interaction) {
>  struct termios tty;
>  if (tcgetattr (0, ) == 0) {
>  oldtty = tty;
> @@ -4328,13 +4328,13 @@ int main(int argc, char **argv)
>  
>  show_banner(argc, argv, options);
>  

> -term_init();
> -
>  /* parse options and open all input/output files */
>  ret = ffmpeg_parse_options(argc, argv);
>  if (ret < 0)
>  exit_program(1);
>  
> +term_init();
> +

I am really sorry: in my last mail, I made a mistake. For my excuses, the
function names are misleading: term_init() does not only init the term, it
also sets the signal handlers; and ffmpeg_parse_options() does not only
parse the options, it also open the files.

That means that, unlike what I wrote, moving term_init() below
ffmpeg_parse_options() will change the behaviour: opening the files will
happen without signal handlers.

Still, initing the term after really parsing the options instead of
out-of-order parsing is better. In particular, regular parsing will set
stdin_interaction to 0 when stdin is used as input, this would not be taken
into account with out-of-order parsing.

I think the best solution would be to split signal initing from term_init()
into a separate signals_init(): then keep the call to signals_init() at the
current place of term_init() and the actual call to term_init() where you
moved it.

>  if (nb_output_files <= 0 && nb_input_files == 0) {
>  show_usage();
>  av_log(NULL, AV_LOG_WARNING, "Use -h to get full help or, even 
> better, run 'man %s'\n", program_name);

Sorry for the wasted time.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Rodger Combs]

---
 ffmpeg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index d858407..08a7a3d 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
 void term_init(void)
 {
 #if HAVE_TERMIOS_H
-if(!run_as_daemon){
+if (!run_as_daemon && stdin_interaction) {
 struct termios tty;
 if (tcgetattr (0, ) == 0) {
 oldtty = tty;
@@ -4328,13 +4328,13 @@ int main(int argc, char **argv)
 
 show_banner(argc, argv, options);
 
-term_init();
-
 /* parse options and open all input/output files */
 ret = ffmpeg_parse_options(argc, argv);
 if (ret < 0)
 exit_program(1);
 
+term_init();
+
 if (nb_output_files <= 0 && nb_input_files == 0) {
 show_usage();
 av_log(NULL, AV_LOG_WARNING, "Use -h to get full help or, even better, 
run 'man %s'\n", program_name);
-- 
2.10.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Nicolas George]

Le tridi 23 fructidor, an CCXXIV, Rodger Combs a écrit :
> Agreed in principle; I did it this way because I'm not entirely sure if
> there would be negative consequences to parsing options (and thus opening
> I/O) before setting up signal handlers. If you think that's not an issue,
> I'll make the change you suggest.

I really do not see anything that could be an issue there, the calls are
very near to each other and options parsing does not use the terminal as
input.

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Rodger Combs]

> On Sep 8, 2016, at 07:49, Nicolas George  wrote:
> 
> Le duodi 22 fructidor, an CCXXIV, Rodger Combs a écrit :
>> ---
>> ffmpeg.c | 6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/ffmpeg.c b/ffmpeg.c
>> index d858407..1d793fe 100644
>> --- a/ffmpeg.c
>> +++ b/ffmpeg.c
>> @@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
>> void term_init(void)
>> {
>> #if HAVE_TERMIOS_H
>> -if(!run_as_daemon){
>> +if (!run_as_daemon && stdin_interaction) {
>> struct termios tty;
>> if (tcgetattr (0, ) == 0) {
>> oldtty = tty;
>> @@ -4328,6 +4328,10 @@ int main(int argc, char **argv)
>> 
>> show_banner(argc, argv, options);
>> 
> 
>> +ret = locate_option(argc, argv, options, "stdin");
>> +if (ret && !strcmp(argv[ret], "-nostdin"))
>> +stdin_interaction = 0;
>> +
>> term_init();
> 
> I think it would be more elegant to move the term_init() call a few lines
> down, after ffmpeg_parse_options(), rather than parse -nostdin out of order.

Agreed in principle; I did it this way because I'm not entirely sure if there 
would be negative consequences to parsing options (and thus opening I/O) before 
setting up signal handlers. If you think that's not an issue, I'll make the 
change you suggest.

> 
> Apart from that, both patches looks right to me.
> 
>> 
>> /* parse options and open all input/output files */
> 
> Regards,
> 
> -- 
>  Nicolas George
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org 
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel 
> 
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Nicolas George]

Le duodi 22 fructidor, an CCXXIV, Rodger Combs a écrit :
> ---
>  ffmpeg.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/ffmpeg.c b/ffmpeg.c
> index d858407..1d793fe 100644
> --- a/ffmpeg.c
> +++ b/ffmpeg.c
> @@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
>  void term_init(void)
>  {
>  #if HAVE_TERMIOS_H
> -if(!run_as_daemon){
> +if (!run_as_daemon && stdin_interaction) {
>  struct termios tty;
>  if (tcgetattr (0, ) == 0) {
>  oldtty = tty;
> @@ -4328,6 +4328,10 @@ int main(int argc, char **argv)
>  
>  show_banner(argc, argv, options);
>  

> +ret = locate_option(argc, argv, options, "stdin");
> +if (ret && !strcmp(argv[ret], "-nostdin"))
> +stdin_interaction = 0;
> +
>  term_init();

I think it would be more elegant to move the term_init() call a few lines
down, after ffmpeg_parse_options(), rather than parse -nostdin out of order.

Apart from that, both patches looks right to me.

>  
>  /* parse options and open all input/output files */

Regards,

-- 
  Nicolas George


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Michael Niedermayer]

On Wed, Sep 07, 2016 at 07:17:59PM -0500, Rodger Combs wrote:
> ---
>  ffmpeg.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)

ill leave these 2 patches for nicolas to review ...

thx

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Asymptotically faster algorithms should always be preferred if you have
asymptotical amounts of data


signature.asc
Description: Digital signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] ffmpeg: don't reconfigure terminal if we're not taking input from stdin

[Rodger Combs]

---
 ffmpeg.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ffmpeg.c b/ffmpeg.c
index d858407..1d793fe 100644
--- a/ffmpeg.c
+++ b/ffmpeg.c
@@ -366,7 +366,7 @@ static BOOL WINAPI CtrlHandler(DWORD fdwCtrlType)
 void term_init(void)
 {
 #if HAVE_TERMIOS_H
-if(!run_as_daemon){
+if (!run_as_daemon && stdin_interaction) {
 struct termios tty;
 if (tcgetattr (0, ) == 0) {
 oldtty = tty;
@@ -4328,6 +4328,10 @@ int main(int argc, char **argv)
 
 show_banner(argc, argv, options);
 
+ret = locate_option(argc, argv, options, "stdin");
+if (ret && !strcmp(argv[ret], "-nostdin"))
+stdin_interaction = 0;
+
 term_init();
 
 /* parse options and open all input/output files */
-- 
2.9.3

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel