[FFmpeg-devel] [PATCH] deprecate SAMPLE_FMT_S24

[Aurelien Jacobs]

Peter Ross wrote:

> On Tue, Aug 19, 2008 at 03:31:52PM +0200, Michael Niedermayer wrote:
> > On Tue, Aug 19, 2008 at 10:29:32PM +1000, Peter Ross wrote:
> > > On Sun, Aug 17, 2008 at 05:10:39PM +0200, Michael Niedermayer wrote:
> > > > On Sun, Aug 17, 2008 at 04:37:49PM +0200, Aurelien Jacobs wrote:
> > > > > Michael Niedermayer wrote:
> > > > > 
> > > > > > On Sun, Aug 17, 2008 at 03:32:30PM +0200, Aurelien Jacobs wrote:
> > > > > > > Peter Ross wrote:
> > > > > > > 
> > > > > > > > This patch flags SAMPLE_FMT_S24 as deprecated.
> > > > > > > 
> > > 
> > > > bits_per_sample can NOT be used as SAMPLE_FMT_* bit count.
> > > > It has a different and docuented purpose, it describes the coded bitstream
> > > > and not the decoded samples, they match in bits only for lossless codecs.
> > > > 
> > > > example:
> > > > the a demuxer sets bits_per_sample to 4 and passes the data to a ADPCM
> > > > decoder which has SAMPLE_FMT_S16 output with 13 significant bits the
> > > > last 3 being always 0
> > > 
> > > Thanks for taking the time to explain this. Revised patch within.
> > > 
> > > The proposed field make sense only for SAMPLE_FMT_S32. I've kept the language
> > > generic, as it will have future utility when FFmpeg can manipulate 12/16-bit
> > > luma planes.
> > 
> > i think that maybe a
> > bits_per_coded_sample   // the number of bits in which each sample is coded
> > and
> > bits_per_raw_sample     // the number of bits in each PCM sample
> 
> Yes. Non-ambiguity is a desirable property. Done.

Agree.

> > I wonder if anyone would complain if we just renamed bits_per_sample to
> > bits_per_coded_sample ? It doesnt break ABI, just API of apps accessing it
> > without AVOptions and apps really should be using AVOptions
> 
> IMHO it should be guarded with an ifdef until the next major version bump.
> Patch enclosed.

I somewhat disagree here. Changing the field name don't breaks ABI, so no
need to make it conditional to version bump.
Instead you could just add a compatibility #define. See below patch
(hand edited, untested...)

Index: libavcodec/utils.c
===================================================================
--- libavcodec/utils.c	(revision 14875)
+++ libavcodec/utils.c	(working copy)
@@ -568,7 +568,10 @@
 {"ec", "set error concealment strategy", OFFSET(error_concealment), FF_OPT_TYPE_FLAGS, 3, INT_MIN, INT_MAX, V|D, "ec"},
 {"guess_mvs", "iterative motion vector (MV) search (slow)", 0, FF_OPT_TYPE_CONST, FF_EC_GUESS_MVS, INT_MIN, INT_MAX, V|D, "ec"},
 {"deblock", "use strong deblock filter for damaged MBs", 0, FF_OPT_TYPE_CONST, FF_EC_DEBLOCK, INT_MIN, INT_MAX, V|D, "ec"},
+#if LIBAVCODEC_VERSION_MAJOR < 52
-{"bits_per_sample", NULL, OFFSET(bits_per_sample), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX},
+{"bits_per_sample", NULL, OFFSET(bits_per_coded_sample), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX},
+#endif
+{"bits_per_coded_sample", NULL, OFFSET(bits_per_coded_sample), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX},
 {"pred", "prediction method", OFFSET(prediction_method), FF_OPT_TYPE_INT, DEFAULT, INT_MIN, INT_MAX, V|E, "pred"},
 {"left", NULL, 0, FF_OPT_TYPE_CONST, FF_PRED_LEFT, INT_MIN, INT_MAX, V|E, "pred"},
 {"plane", NULL, 0, FF_OPT_TYPE_CONST, FF_PRED_PLANE, INT_MIN, INT_MAX, V|E, "pred"},
Index: libavcodec/avcodec.h
===================================================================
--- libavcodec/avcodec.h	(revision 14875)
+++ libavcodec/avcodec.h	(working copy)
@@ -1430,7 +1430,10 @@
      * - encoding: Set by libavcodec.
      * - decoding: Set by user.
      */
-     int bits_per_sample;
+#if LIBAVCODEC_VERSION_MAJOR < 52
+#define bits_per_sample bits_per_coded_sample
+#endif
+     int bits_per_coded_sample;
 
     /**
      * prediction method (needed for huffyuv)

Aurel