[FFmpeg-devel] [PATCH] LPCM in MPEG-TS, next iteration (Cosmetic changes for coding style)

Reimar Döffinger Reimar.Doeffinger
Fri Aug 14 19:09:36 CEST 2009


On Fri, Aug 14, 2009 at 06:54:39PM +0300, Christian P. Schmidt wrote:
> +/**
> + * Static initialization of the Blu-ray PCM codec
> + * @param avctx the codec context
> + */
> +static av_cold int pcm_bluray_decode_init(AVCodecContext *avctx)
> +{
> +    avctx->sample_fmt = SAMPLE_FMT_NONE; ///< Will be determined from the header
> +    return 0;
> +}

Is this necessary?

> +    const uint8_t bits_per_samples[4] = { 0, 16, 20, 24 };
> +    const int64_t channel_layouts[16] = {
> +        0, CH_LAYOUT_MONO, 0, CH_LAYOUT_STEREO, CH_LAYOUT_SURROUND,
> +        CH_LAYOUT_2_1, CH_LAYOUT_4POINT0, CH_LAYOUT_2_2, CH_LAYOUT_5POINT0,
> +        CH_LAYOUT_5POINT1, CH_LAYOUT_7POINT0, CH_LAYOUT_7POINT1, 0, 0, 0, 0
> +    };
> +    const uint8_t channels[16] = {
> +        0, 1, 0, 2, 3, 3, 4, 4, 5, 6, 7, 8, 0, 0, 0, 0
> +    };
> +    const int sample_rates[16] = {
> +        0, 48000, 0, 0, 96000, 192000, 0, 0,
> +        0, 0, 0, 0, 0, 0, 0, 0
> +    };

I think these also should be static.
Also why is channel_layouts 64 bits and signed?
A switch-case would make slightly more sense for sample_rates, too.

> +    /* get the sample depth and derive the sample format from it */
> +    avctx->bits_per_coded_sample = bits_per_samples[(header[3] >> 6) & 3];
> +    if (!avctx->bits_per_coded_sample) {
> +        av_log(avctx, AV_LOG_ERROR, "unsupported sample depth\n");
> +        return -1;
> +    }
> +    avctx->sample_fmt = (avctx->bits_per_coded_sample == 16) ? SAMPLE_FMT_S16 :
> +                                                               SAMPLE_FMT_S32;

unnecessary ()

> +    /* get the sample rate. Not all values are known or exist. */
> +    avctx->sample_rate = sample_rates[header[2] & 0x0f];
> +    if (!avctx->sample_rate) {
> +        av_log(avctx, AV_LOG_ERROR, "unsupported sample rate (%d)\n",
> +               header[2] & 0x0f);
> +        return -1;
> +    }
> +
> +    /*
> +     * get the channel number (and mapping). Not all values are known or exist.
> +     * It must be noted that the number of channels in the MPEG stream can
> +     * differ from the actual meaningful number, e.g. mono audio still has two
> +     * channels, one being empty.
> +     */
> +    avctx->channel_layout = channel_layouts[(header[2] >> 4) & 0x0f];
> +    avctx->channels = channels[(header[2] >> 4) & 0x0f];
> +    if (!avctx->channels) {
> +        av_log(avctx, AV_LOG_ERROR, "unsupported channel configuration (%d)\n",
> +               (header[2] >> 4) & 0x0f);
> +        return -1;
> +    }

A separate variable instead of duplicating or triplicating the bit
extraction code would be quite a but nice.

> +/**
> + * Decode a LPCM packet read from a HDMV MPEG-TS stream
> + * @param avctx the codec context
> + * @param data pointer to the target buffer for decoded audio
> + * @param data_size available space on input, number of decoded bytes on output
> + * @param avpkt the encoded packet
> + */

Doesn't really make sense to document a API function, that is documented
elsewhere already and you just risk having two conflicting
documentations this way.
Also the "read from a HDMV MPEG-TS stream" part is a bit silly, it's not
really going to break if someone added MKV support for this format and it would
be read from an MKV file (at least I very much hope so, otherwise it
would be a very, very broken decoder).

> +    /* There's always an even number of channels in the source */
> +    num_source_channels = (avctx->channels & 1) ? avctx->channels + 1 :
> +                                                  avctx->channels;

FFALIGN

> +    /* Order of calculation matters: early division by 8 kills the fraction for 20bit samples */
> +    sample_size = (num_source_channels * avctx->bits_per_coded_sample) >> 3;
> +    num_samples = buf_size / sample_size;
> +
> +    dprintf(avctx,
> +            "pcm_bluray_decode_frame: c: %d sc: %d s: %d in: %d ds: %d\n",
> +            avctx->channels, num_source_channels, num_samples, buf_size,
> +            *data_size);
> +
> +    switch (avctx->channel_layout) {
> +    case CH_LAYOUT_STEREO:  ///< same number of source and coded channels
> +    case CH_LAYOUT_4POINT0:
> +    case CH_LAYOUT_2_2:
> +        if (16 == avctx->bits_per_coded_sample) {

It would seem more logical to me to check on sample_fmt...

> +#if HAVE_BIGENDIAN
> +            memcpy(dst16, src, 2 * num_samples * num_source_channels);
> +#else
> +            for (sample = num_samples * avctx->channels; sample; sample--)
> +                *dst16++ = bytestream_get_be16(&src);
> +#endif

Which one is it, num_source_channels or avctx->channels? Should be the
same here, but it's no good to do the same thing two ways.
Also on little-endian memcpy+dsputil.swab (does not exist yet) would
possible be better, at least nicer to read.

> +        } else {
> +            for (sample = num_samples * avctx->channels; sample; sample--)
> +                *dst32++ = bytestream_get_be24(&src) << 8;
> +        }

A variable for num_samples * avctx->channels might be a good idea, given
how often that gets calculated.

> +            sample_size = avctx->channels * 2;
> +            for (sample = num_samples; sample; sample--) {
> +#if HAVE_BIGENDIAN
> +                memcpy(dst16, src, sample_size);
> +                dst16 += avctx->channels;
> +#else
> +                for (channel = avctx->channels; channel; channel--)
> +                    *dst16++ = bytestream_get_be16(&src);
> +#endif

That doesn't match at all. Another reason to prefer memcpy+swab on
little-endian, it won't leave the big-endian code completely untested.

> +                for (channel = avctx->channels; channel; channel--)
> +                    *dst32++ = bytestream_get_be24(&src) << 8;

Btw. I doubt this for loop is a very good idea, since it runs downward
the compiler might have less optimizations for it, and without
optimizing it it is not optimal.
With stupid compilers this should give much better results:
> channel = avctx->channels;
> do {
> ...
> } while (--channel);

The compiler could make your variant almost as efficient, but it would
have to add an extra check for avctx->channels == 0 and that check would
probably end up inside the outer loop still.

> +                src += 3;
> +            }
> +        }
> +        break;
> +    case CH_LAYOUT_5POINT1: ///< remapping: L, R, C, LBack, RBack, LF
> +        if (16 == avctx->bits_per_coded_sample) {
> +            for (sample = num_samples; sample; sample--) {
> +                dst16[0] = bytestream_get_be16(&src);
> +                dst16[1] = bytestream_get_be16(&src);
> +                dst16[2] = bytestream_get_be16(&src);
> +                dst16[4] = bytestream_get_be16(&src);
> +                dst16[5] = bytestream_get_be16(&src);
> +                dst16[3] = bytestream_get_be16(&src);
> +                dst16 += 6;
> +            }
> +        } else {
> +            for (sample = num_samples; sample; sample--) {
> +                dst32[0] = bytestream_get_be24(&src) << 8;
> +                dst32[1] = bytestream_get_be24(&src) << 8;
> +                dst32[2] = bytestream_get_be24(&src) << 8;
> +                dst32[4] = bytestream_get_be24(&src) << 8;
> +                dst32[5] = bytestream_get_be24(&src) << 8;
> +                dst32[3] = bytestream_get_be24(&src) << 8;
> +                dst32 += 6;
> +            }
> +        }
> +        break;
> +    case CH_LAYOUT_7POINT0: ///< remapping: L, R, C, LSide, LBack, RBack, RSide, <unused>
> +        if (16 == avctx->bits_per_coded_sample) {
> +            for (sample = num_samples; sample; sample--) {
> +                dst16[0] = bytestream_get_be16(&src);
> +                dst16[1] = bytestream_get_be16(&src);
> +                dst16[2] = bytestream_get_be16(&src);
> +                dst16[5] = bytestream_get_be16(&src);
> +                dst16[3] = bytestream_get_be16(&src);
> +                dst16[4] = bytestream_get_be16(&src);
> +                dst16[6] = bytestream_get_be16(&src);
> +                dst16 += 7;
> +                src += 2;
> +            }
> +        } else {
> +            for (sample = num_samples; sample; sample--) {
> +                dst32[0] = bytestream_get_be24(&src) << 8;
> +                dst32[1] = bytestream_get_be24(&src) << 8;
> +                dst32[2] = bytestream_get_be24(&src) << 8;
> +                dst32[5] = bytestream_get_be24(&src) << 8;
> +                dst32[3] = bytestream_get_be24(&src) << 8;
> +                dst32[4] = bytestream_get_be24(&src) << 8;
> +                dst32[6] = bytestream_get_be24(&src) << 8;
> +                dst32 += 7;
> +                src += 3;
> +            }
> +        }
> +        break;
> +    case CH_LAYOUT_7POINT1: ///< remapping: L, R, C, LSide, LBack, RBack, RSide, LF
> +        if (16 == avctx->bits_per_coded_sample) {
> +            for (sample = num_samples; sample; sample--) {
> +                dst16[0] = bytestream_get_be16(&src);
> +                dst16[1] = bytestream_get_be16(&src);
> +                dst16[2] = bytestream_get_be16(&src);
> +                dst16[6] = bytestream_get_be16(&src);
> +                dst16[4] = bytestream_get_be16(&src);
> +                dst16[5] = bytestream_get_be16(&src);
> +                dst16[7] = bytestream_get_be16(&src);
> +                dst16[3] = bytestream_get_be16(&src);
> +                dst16 += 8;
> +            }
> +        } else {
> +            for (sample = num_samples; sample; sample--) {
> +                dst32[0] = bytestream_get_be24(&src) << 8;
> +                dst32[1] = bytestream_get_be24(&src) << 8;
> +                dst32[2] = bytestream_get_be24(&src) << 8;
> +                dst32[6] = bytestream_get_be24(&src) << 8;
> +                dst32[4] = bytestream_get_be24(&src) << 8;
> +                dst32[5] = bytestream_get_be24(&src) << 8;
> +                dst32[7] = bytestream_get_be24(&src) << 8;
> +                dst32[3] = bytestream_get_be24(&src) << 8;
> +                dst32 += 8;
> +            }
> +        }
> +        break;
> +    }
> +
> +    if (16 == avctx->bits_per_coded_sample)
> +        *data_size = sizeof(int16_t) * num_samples * avctx->channels;
> +    else
> +        *data_size = sizeof(int32_t) * num_samples * avctx->channels;

Above you hardcoded the values 2 and 4 and here you use sizeof, go for
one way to do it.
Also
*data_size = num_samples * avctx->channels * (sample_fmt == SAMPLE_FMT_S32 ? 4 : 2);
duplicates less code.

> diff -urN ffmpeg.old/trunk/libavcodec/pcm-mpeg.c~ ffmpeg/trunk/libavcodec/pcm-mpeg.c~
> --- ffmpeg.old/trunk/libavcodec/pcm-mpeg.c~	1970-01-01 03:00:00.000000000 +0300
> +++ ffmpeg/trunk/libavcodec/pcm-mpeg.c~	2009-08-14 17:48:00.206908666 +0300

?? I'd suggest "svn diff" as a better way to create patches that will
avoid such mistakes (in addition to having a look over patches before
sending them).



More information about the ffmpeg-devel mailing list