[FFmpeg-devel] [PATCH] LPCM in MPEG-TS, next iteration

Christian P. Schmidt schmidt
Tue Aug 25 13:56:19 CEST 2009


Michael Niedermayer wrote:

>> +static int pcm_bluray_decode_frame(AVCodecContext *avctx,
>> +                                   void *data,
>> +                                   int *data_size,
>> +                                   AVPacket *avpkt)
>> +{
>> +    const uint8_t *src = avpkt->data;
>> +    int buf_size = avpkt->size;
>> +    int num_source_channels, channel, retval;
>> +    int sample_size, samples;
> 
>> +#if HAVE_BIGENDIAN
>> +    int used_sample_size;
>> +#endif
> 
> redundant #if

It would give an "unused variable" warning together with below's code:

> 
>> +    int16_t *dst16 = data;
>> +    int32_t *dst32 = data;
>> +
>> +    if (buf_size < 4) {
>> +        av_log(avctx, AV_LOG_ERROR, "PCM packet too small\n");
>> +        return -1;
>> +    }
>> +
>> +    if (pcm_bluray_parse_header(avctx, src))
>> +        return -1;
>> +    src += 4;
>> +    buf_size -= 4;
>> +
>> +    /* There's always an even number of channels in the source */
>> +    num_source_channels = FFALIGN(avctx->channels, 2);
>> +    sample_size = (num_source_channels * avctx->bits_per_coded_sample) >> 3;
>> +    samples = buf_size / sample_size;
>> +    *data_size = samples * avctx->channels *
>> +                 (avctx->sample_fmt == SAMPLE_FMT_S32 ? 4 : 2);
> 
> should there not be a check on data_size being large enough?

Added.

>> +        case CH_LAYOUT_5POINT0:
>> +            if (SAMPLE_FMT_S16 == avctx->sample_fmt) {

>> +#if HAVE_BIGENDIAN
>> +                used_sample_size = avctx->channels * 2;
>> +                do {
>> +                    memcpy(dst16, src, used_sample_size);
>> +                    dst16 += avctx->channels;
>> +                    src += sample_size;
>> +                } while (--samples);
>> +#else
>> +                do {
>> +                    channel = avctx->channels;
>> +                    do {
>> +                        *dst16++ = bytestream_get_be16(&src);
>> +                    } while (--channel);
>> +                    src += 2;
>> +                } while (--samples);
>> +#endif
> 
> ugly, duplicated and factorizeable

I do kind of agree. Alternative code (b) doing the same would however
look even worse, e.g.:

#if HAVE_BIGENDIAN
                used_sample_size = avctx->channels * 2;
#endif
                do {
#if HAVE_BIGENDIAN
                    memcpy(dst16, src, used_sample_size);
                    dst16 += avctx->channels;
                    src += sample_size;
#else
                    channel = avctx->channels;
                    do {
                        *dst16++ = bytestream_get_be16(&src);
                    } while (--channel);
                    src += 2;
#endif
                } while (--samples);

On extra #ifdef block to keep only one do...while loop.

The next option (c) would be to remove the #ifdefs for used_sample_size,
both in the declaration and the assignment, and hope that the compiler
realizes the calculated value is never used and completely removes it on
little endian machines, or in worst case live with the unnecessary
calculation once per audio frame.

The final option (d) would be to remove used_sample_size completely and
just use
                do {
#if HAVE_BIGENDIAN
                    memcpy(dst16, src, avctx->channels * 2);
                    dst16 += avctx->channels;
                    src += sample_size;
#else
                    channel = avctx->channels;
                    do {
                        *dst16++ = bytestream_get_be16(&src);
                    } while (--channel);
                    src += 2;
#endif
                } while (--samples);

and hope the compiler caches the calculated value inside the memcpy(). I
personally think any change of the original code would rely on the
compiler doing the right thing, or look even worse.

Which of the options would be preferred? I'll post a complete updated
patch once this part is decided.

Regards,
Christian



More information about the ffmpeg-devel mailing list