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

[Lars Täuber]

Hallo!

On Thu, 17 Apr 2008 21:10:07 +0100 M?ns Rullg?rd <mans at mansr.com> wrote:
> Lars T?uber <lars.taeuber at gmx.net> writes:
> > 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.

I don't know if the current version is better, because I'm not sure if I understood you.
 
> > +        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.

Changed.
 
> > 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--)

Changed.
 
> Wrong indentation.

Corrected.

> > +                ap =  audio24;
> 
> Extra space after =.

Changed.

> > +                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.

Changed.
 
> > +                    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 ().

I hope I did it the right way, because I'm not sure about the precedence rules.

> > +                    }
> > +                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?

I'd like to work on a patch that makes ffmpeg support more than 16 bit for audio after this has been accepted.
For instance to convert 24bit pcm_dvd to 24bit flac. But I don't know how to do this right now.
Is this overhead acceptable till then?

> Also, properly rounding the
> values to 16 bits (rather than truncating) might be preferable.

Is the current version proper rounding? Or is there a ff_* function available somewhere?

> > +            }
> > +        }
> 
> Indentation doesn't match opening brace.

Changed.
 
> M?ns Rullg?rd
> mans at mansr.com

Best regards.
Lars

PS: I'm not a software developer not even a programer.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcm_dvd.Makefile.diff
Type: text/x-diff
Size: 606 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080417/ec44c001/attachment.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcm_dvd.mpeg.diff
Type: text/x-diff
Size: 1471 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080417/ec44c001/attachment-0001.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pcm_dvd.pcm.diff
Type: text/x-diff
Size: 2604 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080417/ec44c001/attachment-0002.diff>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sanity_check.pcm.diff
Type: text/x-diff
Size: 1127 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080417/ec44c001/attachment-0003.diff>