[FFmpeg-devel] [PATCH] Add documentation for -ast, -vst, -sst

[Michael Niedermayer]

On Sat, Feb 28, 2009 at 12:57:02AM +0100, Stefano Sabatini wrote:
> On date Friday 2009-02-27 17:19:30 -0500, Ronald S. Bultje encoded:
> > Hi,
> > 
> > On Fri, Feb 27, 2009 at 4:18 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > On Fri, Feb 27, 2009 at 03:26:19PM -0500, Ronald S. Bultje wrote:
> > >> On Thu, Feb 26, 2009 at 2:43 PM, Stefano Sabatini
> > >> <stefano.sabatini-lala at poste.it> wrote:
> > >> > On date Tuesday 2009-02-24 23:50:40 +0100, Stefano Sabatini encoded:
> > >> >> + at item -ast @var{audio_stream_number}
> > >> >> +Select the desired audio stream number, counting from 1.
> > >>
> > >> This has confused me all too often. Can we change this so it counts
> > >> like a regular number from 0?
> > >
> > > yes
> > 
> > that piece of code is full of crap btw, <0 for subtitles is seen as
> > "disable" but <0 for audio/video isn't. I added that also, which makes
> > the subtitle/video/audio selection code more alike. This would also
> > allow to remove -an/-vn at some point but that could also be done
> > after the release.
> > 
> > if you think this changes too much, I'll just apply the
> > "wanted_x_stream < 0" to "wanted_x_stream <= 0" *3 change.
> > 
> > Stefano, can you regen your documentation patch with this change taken
> > into account and apply that also?
> 
> Hi, thanks for noticing the issue, yes the code as it is is greatly
> obfuscated and subtlely broken.
> 
> I split your patch and done more simplifications.
> Summary:
> 
> * ffplay-fix-sst-logic.patch
>   When the user selects the subtitle stream N, if N is not 0 then it
>   is always selected N-1.
>   Patches fixes this.
> 
> * ffplay-simplify-sst-logic.patch
>   Simplify the logic, using the following reasoning:
> 
>   A & B & (C | A) =
>   A & B
> 
>   since if A & B then A is always true so (C | A) is always true.
>   Formal derivation below:
> 
>   A & B & (C | A) =
>   (A & B) & (C | A) =
>   (A & B & C) | (A & B & A) =
>   (A & B & C) | (A & B) =
>   A & B
> 
>   Thus the third condition (subtitle_index < 0) is unnecessary.
> 
> * ffplay-consistent-ast-vst.patch
>   Make the -ast and -vst behaviour consistent with that of -sst, that
>   is count starts from 0 and negative values are interpreted as
>   "disable".
> 
> * ffplay-document-xst.patch
>   Document the -ast, -vst, -sst options according to the new
>   (sane) behaviour.
> 
> Regards.
> -- 
> FFmpeg = Fundamentalist and Faboulous Murdering Portentous Erroneous Gem

> Index: ffmpeg/ffplay.c
> ===================================================================
> --- ffmpeg.orig/ffplay.c	2009-02-28 00:08:26.000000000 +0100
> +++ ffmpeg/ffplay.c	2009-02-28 00:31:10.000000000 +0100
> @@ -1996,7 +1996,7 @@
>              break;
>          case CODEC_TYPE_SUBTITLE:
>              if (wanted_subtitle_stream >= 0 && !video_disable &&
> -                    (subtitle_index < 0 || wanted_subtitle_stream-- > 0))
> +                    (subtitle_index < 0 || wanted_subtitle_stream-- >= 0))
>                  subtitle_index = i;
>              break;
>          default:

ok


> Index: ffmpeg/ffplay.c
> ===================================================================
> --- ffmpeg.orig/ffplay.c	2009-02-28 00:41:39.000000000 +0100
> +++ ffmpeg/ffplay.c	2009-02-28 00:45:00.000000000 +0100
> @@ -1995,8 +1995,7 @@
>                  video_index = i;
>              break;
>          case CODEC_TYPE_SUBTITLE:
> -            if (wanted_subtitle_stream >= 0 && !video_disable &&
> -                    (subtitle_index < 0 || wanted_subtitle_stream-- >= 0))
> +            if (wanted_subtitle_stream-- >= 0 && !video_disable)
>                  subtitle_index = i;
>              break;
>          default:

this patch is incorrect
but thanks for optimizing the bug


> Index: ffmpeg/ffplay.c
> ===================================================================
> --- ffmpeg.orig/ffplay.c	2009-02-28 00:45:00.000000000 +0100
> +++ ffmpeg/ffplay.c	2009-02-28 00:46:11.000000000 +0100
> @@ -1987,11 +1987,11 @@
>          ic->streams[i]->discard = AVDISCARD_ALL;
>          switch(enc->codec_type) {
>          case CODEC_TYPE_AUDIO:
> -            if ((audio_index < 0 || wanted_audio_stream-- > 0) && !audio_disable)
> +            if (wanted_audio_stream-- >= 0 && !audio_disable)
>                  audio_index = i;
>              break;
>          case CODEC_TYPE_VIDEO:
> -            if ((video_index < 0 || wanted_video_stream-- > 0) && !video_disable)
> +            if (wanted_video_stream-- >= 0 && !video_disable)
>                  video_index = i;
>              break;
>          case CODEC_TYPE_SUBTITLE:

these require the default values to be changed to 0 or you break the code
entirely

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

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090228/b743f9a6/attachment.pgp>