On 06/05/2011 06:31 PM, Stefano Sabatini wrote:
> 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()
I think we could do both. 1 is preferable and would be nice to have for
most cases, but we can use 2 in the cases where it's needed before the
decoder is opened. Also, 2 might be useful for other applications. And
I do like the name avcodec_get_bits_per_coded_sample().
-Justin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
