[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