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

[Michael Niedermayer]

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

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

Opposition brings concord. Out of discord comes the fairest harmony.
-- Heraclitus
-------------- 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/df189d1b/attachment.sig>