[Ffmpeg-devel] [PATCH] support S302M streams in MPEG TS

Baptiste Coudurier baptiste.coudurier
Tue Dec 12 18:58:08 CET 2006


Hi

Reimar D?ffinger wrote:
> [...]
> 
>> +static void s302m_parse_vucf_bits(S302MContext *s, const uint8_t *buf)
>> +{
>> +    uint8_t vucf[2];
>> +    if (s->avctx->bits_per_sample == 16) {
>> +        s->avctx->codec_id = CODEC_ID_PCM_S16BE;
>> +        vucf[0] = (buf[2] & 0xf0) | (buf[4] & 0x0f);
>> +        vucf[1] = (buf[7] & 0xf0) | (buf[9] & 0x0f);
>> +    } else if (s->avctx->bits_per_sample == 20) {
>> +        /* store 20 bit samples on 24 bits */
>> +        s->avctx->bits_per_sample = 24;
> 
> IMO it would be cleaner to separate the real bits per sample and those
> exported instead of overwriting it. E.g. we might for some reason want
> to call s302m_parse_vucf_bits more than once some time in the future..

Ok.

>> +    if ((vucf[0] >> 6) & 0x01)
> 
> You mean (vucf[0] & 0x40) ?

I think.

>> +    if (((vucf[1] >> 4) & 0x0f) > 6)
> 
> unless you can give that 6 a special meaning,
> (vucf[1] > 0x60)
> would be simpler and just as good.

AES3 specs:
Byte 1: bits 0 1 2 3
0 1 1 1 Single channel double sampling frequency mode
1 0 0 0 Single channel double sampling frequency mode
1 0 0 1 Single channel double sampling frequency mode

I'll add comment.

>> +static void s302m_subframe_16bit(S302MContext *s, const uint8_t *buf)
>> +{
>> +    /* reverse sample to suit big endian */
>> +    s->outbuf_ptr[0] = ff_reverse[buf[0]];
>> +    s->outbuf_ptr[1] = ff_reverse[buf[1]];
>> +    s->outbuf_ptr[3] = ff_reverse[((buf[3] & 0x0f) << 4) | (buf[4] >> 4)];
>> +    s->outbuf_ptr[4] = ff_reverse[((buf[2] & 0x0f) << 4) | (buf[3] >> 4)];
>> +    s->outbuf_ptr += 4;
> 
> typo? [2] is not initialized and [4] gets overwritten later...
> And wouldn't it be nicer if these transformations were done in-place,
> with only one buffer?
> 

Right. I don't have any 16 bit sample though.

>> +    s->frame_size = (buf[0] << 8) | buf[1];
>> +    if (s->frame_size > 57360)
> 
> could you add a comment where 57360? And writing it as hex 0xe010 at
> least wouldn't hurt.

Comment is in s302m.h, do you want me to add it there ?

>> +    s->avctx->channels = 2 + ((buf[2] >> 6) & 3) * 2;
> 
> (buf[2] >> 5) & 6? Makes it easier to see the max. Not sure which one is better.

Is it the same ? I only want 2 bits, and * 2 to lookup bits -> channels

>> +    s->avctx->bits_per_sample = 16 + ((buf[3] >> 4) & 3) * 4;
> 
> similarly here (buf[3] >> 2) & 0x0c

Same.

>> +    s->byte_align = ((s->avctx->bits_per_sample + 4) * 2) >> 3;
> 
> maybe it is clearer to just put this into the bits_per_sample ifs?

byte_align is: (bits_per_sample + vucf bits) * samples (2) / 8 which
ensure byte alignment in S302M specs.

>> +            if (!s->avctx->sample_rate)
>> +                s302m_parse_vucf_bits(s, s->inbuf);
> 
> Isn't this misusing sample_rate a bit?

If sample rate is not set, I need to parse vucf bits to set it, if it is
already set, no need to parse vucf bits again.
Same behaviour as has_codec_parameters in libavformat/utils.c

>> +S302MContext *s302m_init_demuxer(AVStream *st)
>> +{
>> +    S302MContext *s = av_mallocz(sizeof(*s));
>> +    s->avctx = st->codec;
>> +    s->avctx->codec_type = CODEC_TYPE_AUDIO;
> 
> Might set some other defaults, too, at least I think there is something
> s302m_parse_vucf_bits sets that does not depend on data? Maybe I just
> misremember.
> 

Humm dunno, like what ?

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
SMARTJOG S.A.                                    http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
Phone: +33 1 49966312




More information about the ffmpeg-devel mailing list