[FFmpeg-devel] [RFC] LPCM 24 bits support

[Måns Rullgård]

Lars T?uber <lars.taeuber at gmx.net> writes:

> Hallo!
>
> On Wed, 16 Apr 2008 23:43:32 +0200 Diego Biurrun <diego at biurrun.de> wrote:
>> On Wed, Apr 16, 2008 at 10:46:39PM +0200, Lars T?uber wrote:
>> > 
>> > On Wed, 16 Apr 2008 22:33:08 +0200 Diego Biurrun <diego at biurrun.de> wrote:
>> > > > --- ffmpeg/libavcodec/Makefile	2008-04-16 18:00:57.000000000 +0200
>> > > > +++ ffmpeg.1/libavcodec/Makefile	2008-04-16 18:04:11.000000000 +0200
>> > > > @@ -270,6 +270,7 @@ OBJS-$(CONFIG_PCM_MULAW_DECODER)       +
>> > > >  OBJS-$(CONFIG_PCM_MULAW_ENCODER)       += pcm.o
>> > > >  OBJS-$(CONFIG_PCM_ZORK_DECODER)        += pcm.o
>> > > >  OBJS-$(CONFIG_PCM_ZORK_ENCODER)        += pcm.o
>> > > > +OBJS-$(CONFIG_PCM_DVD_DECODER)         += pcm.o
>> > > 
>> > > Please keep alphabetical order, same in other places.
>> > 
>> > It's hard to keep an order that doesn't exist.
>> 
>> I can assure you that list above is ordered alphabetically right now.
>> Look again.
>
> I hope I guessed your order right.
> What about the attached patches?
>
> Lars
>
>
> diff -pur ffmpeg/libavformat/mpeg.c ffmpeg.1/libavformat/mpeg.c
> --- ffmpeg/libavformat/mpeg.c	2008-03-05 15:14:32.000000000 +0100
> +++ ffmpeg.1/libavformat/mpeg.c	2008-04-16 10:10:22.000000000 +0200
> @@ -473,7 +473,8 @@ static int mpegps_read_packet(AVFormatCo
>          codec_id = CODEC_ID_DTS;
>      } else if (startcode >= 0xa0 && startcode <= 0xaf) {
>          type = CODEC_TYPE_AUDIO;
> -        codec_id = CODEC_ID_PCM_S16BE;
> +        /* CODEC_ID_PCM_S16BE is a special form of CODEC_ID_PCM_DVD */
> +        codec_id = CODEC_ID_PCM_DVD;

That comment is somewhat inaccurate.

>      } else if (startcode >= 0xb0 && startcode <= 0xbf) {
>          type = CODEC_TYPE_AUDIO;
>          codec_id = CODEC_ID_MLP;
> @@ -518,7 +519,15 @@ static int mpegps_read_packet(AVFormatCo
>          freq = (b1 >> 4) & 3;
>          st->codec->sample_rate = lpcm_freq_tab[freq];
>          st->codec->channels = 1 + (b1 & 7);
> -        st->codec->bit_rate = st->codec->channels * st->codec->sample_rate * 2;

Looks like bit_rate was incorrectly set.

> +        st->codec->bits_per_sample = 16 + ((b1 >> 6) & 3) * 4;
> +        st->codec->bit_rate = st->codec->channels *
> +                              st->codec->sample_rate *
> +                              st->codec->bits_per_sample;

OK, assuming that's what the bits mean.  I have no specs.

> +        if (16 == st->codec->bits_per_sample)
> +            st->codec->codec_id = CODEC_ID_PCM_S16BE;
> +        else if (28 == st->codec->bits_per_sample)
> +            return AVERROR(EINVAL);

I prefer writing bits_per_sample == 16 etc.

>      }
>      av_new_packet(pkt, len);
>      get_buffer(s->pb, pkt->data, pkt->size);
>
>
> diff -pur ffmpeg/libavcodec/allcodecs.c ffmpeg.1/libavcodec/allcodecs.c
> --- ffmpeg/libavcodec/allcodecs.c	2008-04-14 18:39:54.000000000 +0200
> +++ ffmpeg.1/libavcodec/allcodecs.c	2008-04-17 20:24:14.000000000 +0200
> @@ -214,6 +214,7 @@ void avcodec_register_all(void)
>  
>      /* pcm codecs */
>      REGISTER_ENCDEC  (PCM_ALAW, pcm_alaw);
> +    REGISTER_DECODER (PCM_DVD  , pcm_dvd);
>      REGISTER_ENCDEC  (PCM_MULAW, pcm_mulaw);
>      REGISTER_ENCDEC  (PCM_S8, pcm_s8);
>      REGISTER_ENCDEC  (PCM_S16BE, pcm_s16be);
> diff -pur ffmpeg/libavcodec/avcodec.h ffmpeg.1/libavcodec/avcodec.h
> --- ffmpeg/libavcodec/avcodec.h	2008-04-14 18:39:54.000000000 +0200
> +++ ffmpeg.1/libavcodec/avcodec.h	2008-04-17 20:36:18.000000000 +0200
> @@ -207,6 +207,7 @@ enum CodecID {
>      CODEC_ID_PCM_S24DAUD,
>      CODEC_ID_PCM_ZORK,
>      CODEC_ID_PCM_S16LE_PLANAR,
> +    CODEC_ID_PCM_DVD,
>  
>      /* various ADPCM codecs */
>      CODEC_ID_ADPCM_IMA_QT= 0x11000,
> diff -pur ffmpeg/libavcodec/pcm.c ffmpeg.1/libavcodec/pcm.c
> --- ffmpeg/libavcodec/pcm.c	2008-03-21 13:17:05.000000000 +0100
> +++ ffmpeg.1/libavcodec/pcm.c	2008-04-17 20:25:09.000000000 +0200
> @@ -492,6 +498,32 @@ static int pcm_decode_frame(AVCodecConte
>              *samples++ = s->table[*src++];
>          }
>          break;
> +    case CODEC_ID_PCM_DVD:
> +    {
> +        int audio24[8*2], *ap;
> +        const uint8_t *src_LSB;
> +
> +        n = buf_size / (avctx->channels * 2 * avctx->bits_per_sample / 8);
> +            for (; n>0; n--) {

while (n--)

Wrong indentation.

> +                ap =  audio24;

Extra space after =.

> +                src_LSB = src + avctx->channels * 2 * 2;
> +
> +                if (20 == avctx->bits_per_sample)

See above about ==.  I also like { } with multi-line if() bodies, even
if it's only one statement.

> +                    for (c=0; c < avctx->channels; c++, src+=4, src_LSB++ ) {
> +                        *ap++ = (src[0]<<16) + (src[1]<<8) +  (*src_LSB & 0xf0);
> +                        *ap++ = (src[2]<<16) + (src[3]<<8) + ((*src_LSB & 0x0f) << 4);

Could use | instead of + and lose some ().

> +                    }
> +                else if (24 == avctx->bits_per_sample)
> +                    for (c=0; c < 2*avctx->channels; c++, src+=2, src_LSB++ )
> +                        *ap++ = (src[0]<<16) + (src[1]<<8) + *src_LSB;
> +
> +                src = src_LSB;
> +
> +                for (c=0; c < avctx->channels*2; c++)
> +                    *samples++ = audio24[c] >> 8;

What's the point in saving 24 bits per sample to a temporary buffer,
only to discard the low 8 bits later?  Also, properly rounding the
values to 16 bits (rather than truncating) might be preferable.

> +            }
> +        }

Indentation doesn't match opening brace.

> +        break;
>      default:
>          return -1;
>      }
> @@ -551,5 +583,6 @@ PCM_CODEC  (CODEC_ID_PCM_U16BE, pcm_u16b
>  PCM_CODEC  (CODEC_ID_PCM_S8, pcm_s8);
>  PCM_CODEC  (CODEC_ID_PCM_U8, pcm_u8);
>  PCM_CODEC  (CODEC_ID_PCM_ALAW, pcm_alaw);
> +PCM_DECODER(CODEC_ID_PCM_DVD, pcm_dvd);
>  PCM_CODEC  (CODEC_ID_PCM_MULAW, pcm_mulaw);
>  PCM_CODEC  (CODEC_ID_PCM_ZORK, pcm_zork);
>

-- 
M?ns Rullg?rd
mans at mansr.com