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

[Michael Niedermayer]

On Thu, Jul 30, 2015 at 10:58:57PM -0400, Ganesh Ajjanagadde wrote:
> 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?

i dont think this solves the problem, not everything calls ffmpeg
through such wrapper

ATM ffmpeg in practice doesnt leave the terminal in a messed up state
from te users point of view when it crashes. And there should be no
regression here.
I dont think it would be an improvment if #2964 is fixed
and then instead have ffmpeg randomly mess up the terminal instead
2964: (aka the
user can quit a process by pressing 'q' instead of crtl-c when its
stderr is oddly redirected and stderr actually is what the whole
system uses to communicate so the user cannot even see the message
about pressing 'q')

I think #2964 is less important than a regression with messing up the
terminal. So to fix #2964 a solution would need to be found which
does not cause the terminal to be messed up on crashes in real
world cases. I dont know how to do that. And i dont know enough
about this subject to say if its practically possible or not.

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

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150731/53a1c480/attachment.sig>