[FFmpeg-devel] [PATCH] ffmpeg: modify tty state when stderr is redirected

[Ganesh Ajjanagadde]

On Thu, Jul 30, 2015 at 9:53 PM, Michael Niedermayer
<michael at niedermayer.cc> wrote:
> On Thu, Jul 30, 2015 at 05:57:54PM -0400, Ganesh Ajjanagadde wrote:
>> On Thu, Jul 30, 2015 at 5:49 PM, Michael Niedermayer
>> <michael at niedermayer.cc> wrote:
>> > On Thu, Jul 30, 2015 at 02:43:01PM +0200, Michael Niedermayer wrote:
>> >> On Wed, Jul 29, 2015 at 05:28:16PM -0400, Ganesh Ajjanagadde wrote:
>> >> > On Wed, Jul 29, 2015 at 3:27 PM, Michael Niedermayer
>> >> > <michael at niedermayer.cc> wrote:
>> >> > > On Wed, Jul 29, 2015 at 02:43:52PM -0400, Ganesh Ajjanagadde wrote:
>> >> > >> On Mon, Jul 27, 2015 at 9:56 AM, Ganesh Ajjanagadde
>> >> > >> <gajjanagadde at gmail.com> wrote:
>> >> > >> > This fixes Ticket2964
>> >> > >> >
>> >> > >> > Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
>> >> > >> > ---
>> >> > >> >  ffmpeg.c | 2 +-
>> >> > >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> > >> >
>> >> > >> > diff --git a/ffmpeg.c b/ffmpeg.c
>> >> > >> > index 751c7d3..98f812e 100644
>> >> > >> > --- a/ffmpeg.c
>> >> > >> > +++ b/ffmpeg.c
>> >> > >> > @@ -372,7 +372,7 @@ void term_init(void)
>> >> > >> >          struct termios tty;
>> >> > >> >          int istty = 1;
>> >> > >> >  #if HAVE_ISATTY
>> >> > >> > -        istty = isatty(0) && isatty(2);
>> >> > >> > +        istty = isatty(0);
>> >> > >> >  #endif
>> >> > >> >          if (istty && tcgetattr (0, &tty) == 0) {
>> >> > >> >              oldtty = tty;
>> >> > >>
>> >> > >> ping
>> >> > >
>> >> > > i dont mind applying this but i dont remember why it was there
>> >> > > so this might break somethig and someone might then have to revert
>> >> >
>> >> > See the long discussion I had (with my initial patch series) for full details.
>> >> > A short summary is as follows:
>> >> > in order to accept "q" and other stuff, ffmpeg has to change the terminal mode.
>> >> > Once terminal mode is changed, on event of "hard" signal like SIGSEGV,
>> >> > it is not the responsibility of ffmpeg to clean up and restore the
>> >> > terminal state
>> >> > that now appears as messed up.
>> >> > I had a patch to do this, but this requires registering signal handler
>> >> > for such signals,
>> >> > and others had valid objections since the core dump is no longer clean.
>> >> > Thus, terminal restoration should be handled by the shell.
>> >> > Fortunately, zsh has such functionality (thanks Nicolas for pointing
>> >> > this out!) via "ttyctl -f"
>> >> > to "freeze" terminal, i.e prevent any process from damaging the
>> >> > terminal state on exit.
>> >> > In bash it is harder to do this; AFAIK requires manual intervention.
>> >> >
>> >> > Unless fate tests redirect 2(stderr) and do not redirect 0(stdin),
>> >> > functionality is identical.
>> >> > Even otherwise, by above argument, I think this is the right thing to do.
>> >>
>> >> patch applied
>> >>
>> >> note, if something breaks, ill revert this one, but hopefully it
>> >> will work fine
>> >
>> > any failure in fate trashes the terminal, thus reverted
>> >
>> > To reproduce, add a abort() into wav_read_header()
>> > run make fate-acodec-adpcm-ima_wav
>>
>> Yes, but the trashing cleanup is not ffmpeg's job; the shell should do it.
>
> no disagreement here but as the shell does not do that (well at least
> not bash here)
> it causes moderate inconvenience to the developers
> in everyday work if the terminal needs to be reset after a fate
> failure

Ok, in that case I will reopen the ticket.
Note that this rules out straightforward fixes.
The only idea I have left is to create wrapper functions around ffmpeg
invocations (e.g in fate) like:

function ffmpeg_wrapper()
{
    STTYOPTS="$(stty --save)"
    ffmpeg "$@"
    stty "${STTYOPTS}"
}.

What do people think of this?

>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> Opposition brings concord. Out of discord comes the fairest harmony.
> -- Heraclitus
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>