[FFmpeg-devel] [RFC] av_get_bits_per_sample() questions

[Stefano Sabatini]

On date Sunday 2011-03-13 17:20:51 -0400, Justin Ruggles wrote:
> On 03/13/2011 04:59 PM, Stefano Sabatini wrote:
> 
> > Hi,
> > 
> > just discovered that we have:
> > 
> > int av_get_bits_per_sample(enum CodecID codec_id);
> > and:
> > int av_get_bits_per_sample_fmt(enum AVSampleFormat sample_fmt);
> > 
> > av_get_bits_per_sample() return per-codec bits per sample, and is only
> > used in pcm:
> > ./pcm.c:48:    avctx->bits_per_coded_sample = av_get_bits_per_sample(avctx->codec->id);
> > ./pcm.c:93:    sample_size = av_get_bits_per_sample(avctx->codec->id)/8;
> > ./pcm.c:232:        avctx->bits_per_raw_sample = av_get_bits_per_sample(avctx->codec->id);
> > ./pcm.c:285:    sample_size = av_get_bits_per_sample(avctx->codec_id)/8;
> > ./pcm.c:287:    /* av_get_bits_per_sample returns 0 for CODEC_ID_PCM_DVD */
> > 
> > Since this is a per-codec property I wonder if it would be a better
> > idea to put the information in the codec context and drop the
> > function.
> 
> I agree it's ugly.
> 

> I think it's supposed to just be a convenience function for libavformat
> and other muxers/demuxers to be able to handle raw pcm/adpcm/dpcm more
> easily.  I think it would be ok to redefine how bits_per_raw_sample is
> used as an alternative.  Currently it only has meaning for encoding with
> sample_fmt==SAMPLE_FMT_S32, but I think it would be better if that were
> expanded to be valid for any sample format with any raw pcm-like codec
> (ADPCM, DPCM, PCM).  Then I think we could set that in the decoders and
> make the function return bits_per_raw_sample just for backwards
> compatibility.

Is there a specific reason for preferring
AVCodecContext.bits_per_raw_sample over bits_per_coded_sample?, I can't
say exactly which is the difference, the doxy is not particularly helpful:

    /**
     * bits per sample/pixel from the demuxer (needed for huffyuv).
     * - encoding: Set by libavcodec.
     * - decoding: Set by user.
     */
     int bits_per_coded_sample;
...
    /**
     * Bits per sample/pixel of internal libavcodec pixel/sample format.
     * This field is applicable only when sample_fmt is AV_SAMPLE_FMT_S32.
     * - encoding: set by user.
     * - decoding: set by libavcodec.
     */
    int bits_per_raw_sample;


Currently the behavior of av_get_bits_per_sample(codec_id) is to
return a value expressing the compressed sample size for PCM and
ADPCM audio codecs (it will be 0 otherwise).

The function it is sometimes called *before* the codec context is
opened, like in mov.c:ff_mov_read_stsd_entries():

            bits_per_sample = av_get_bits_per_sample(st->codec->codec_id);
            if (bits_per_sample) {
                st->codec->bits_per_coded_sample = bits_per_sample;
                sc->sample_size = (bits_per_sample >> 3) * st->codec->channels;
            }

this code sets sc->sample_size only for AD/PCM codecs, at this stage
the codec was not still initialized so bits_per_coded_sample can't
yet be set in the decoder.

I see different solutions:
1) remove av_get_bits_per_sample(), and directly access
   avctx->bits_per_coded_context, which is set during the decoder
   initialization.

   In the above case we need the property value *before* to
   actually open the decoder, so we may do:

   if (codec id is AD/PCM) {
      bits_per_sample = ff_pcm_get_bits_per_coded_sample(codec_id);
      sc->sample_size = (bits_per_sample >> 3) * st->codec->channels;
   }

   _assuming_ this is indeed the correct semantics (Baptiste?).

2) leave the av_get_bits_per_sample() function, possibly give a more
   meaningful name to it, something like
   avcodec_get_bits_per_coded_sample() for avoiding confusion with
   av_get_bits/bytes_per_sample()