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

Christian P. Schmidt schmidt
Sat Aug 15 01:14:05 CEST 2009


Reimar D?ffinger wrote:
> On Fri, Aug 14, 2009 at 06:54:39PM +0300, Christian P. Schmidt wrote:
>> +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?

I removed the sample_fmt initialization. The function seems necessary as
 ffplay crashed on me without, and with NULL in it's place down in the
codec struct.

>> +    const uint8_t bits_per_samples[4] = { 0, 16, 20, 24 };
>> +    const int64_t channel_layouts[16] = {
>> +    const uint8_t channels[16] = {
>> +    const int sample_rates[16] = {
> 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.

There no need for static as they are inside the function, and noone else
needs them. channel_layout changed to uint32_t, sample rates to a
switch-case construct.

>> +    avctx->sample_fmt = (avctx->bits_per_coded_sample == 16) ? SAMPLE_FMT_S16 :
>> +                                                               SAMPLE_FMT_S32;
> unnecessary ()

Removed. I personally think they aid readability with the ?: operator,
but...

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

Done for this case as two lookups are guaranteed. Also removed the
masking as those bits are exactly what remains after the shift.

>> +/**
>> + * 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).

I doubt the Matroska guys would ever specify multiple PCM formats with
different channel layouts or endianess. Nonetheless, comment removed for
the other reasons.

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

Applied.

>> +        if (16 == avctx->bits_per_coded_sample) {
> It would seem more logical to me to check on sample_fmt...

Doesn't make a difference because sample_fmt is initialized from
bits_per_coded_sample during header parsing, but changed.

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

Unified.

> Also on little-endian memcpy+dsputil.swab (does not exist yet) would
> possible be better, at least nicer to read.

With assembler a lot could be improved, also on the channel reordering part.

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

It's embedded in the restructuring changes. It came pretty naturally
with the do-while-loops.

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

Good spotting, I missed the src increment. While changing the loops I
found even another bug.

>> +                for (channel = avctx->channels; channel; channel--)
>> +                    *dst32++ = bytestream_get_be24(&src) << 8;
> 
> 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.

Changed all loops. The check is done far out now.

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

I took the latter suggestion. The calculation was moved up to remove the
need for another variable.

Regards,
Christian
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: lpcm-mpegts.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090815/d16e99e9/attachment.txt>



More information about the ffmpeg-devel mailing list