[Ffmpeg-devel] [PATCH] apple caff demuxer

[Baptiste Coudurier]

Hi

Justin Ruggles wrote:
>>> [...]
>>> +        if(ctx->bits_per_channel == 8) {
>>> +            ctx->codec_id = CODEC_ID_PCM_S8;
>>> +        } else if(ctx->bits_per_channel == 16) {
>>> +            if(ctx->format_flags & CAFF_LPCM_FLAGS_IS_LITTLEENDIAN) {
>>> +                ctx->codec_id = CODEC_ID_PCM_S16LE;
>>> +            } else {
>>> +                ctx->codec_id = CODEC_ID_PCM_S16BE;
>>> +            }
>>> +        } else if(ctx->bits_per_channel == 24) {
>>> +            if(ctx->format_flags & CAFF_LPCM_FLAGS_IS_LITTLEENDIAN) {
>>> +                ctx->codec_id = CODEC_ID_PCM_S24LE;
>>> +            } else {
>>> +                ctx->codec_id = CODEC_ID_PCM_S24BE;
>>> +            }
>>> +        } else if(ctx->bits_per_channel == 32) {
>>> +            if(ctx->format_flags & CAFF_LPCM_FLAGS_IS_LITTLEENDIAN) {
>>> +                ctx->codec_id = CODEC_ID_PCM_S32LE;
>>> +            } else {
>>> +                ctx->codec_id = CODEC_ID_PCM_S32BE;
>>> +            }
>>
>> that mess begins to be duplicated in every demuxer, maybe
>> CODEC_ID_RAWAUDIO should be added, how to handle the BE/LE case ?
> 
> maybe CODEC_ID_PCM_BE / CODEC_ID_PCM_LE.  and actually use the
> SampleFormat in AVCodecContext, which would make it easy to implement
> floating-point PCM decoding.

CODEC_ID_LPCM_BE/LE, ok with that, any other opinion ?

>>> [...]
>>> +
>>> +    av_set_pts_info(st, 64, 1, st->codec->sample_rate);
>>> +    st->start_time = 0;
>>> +    st->duration = st->nb_frames;
>>
>> wrong, duration is in stream timebase according to demuxers and utils.c,
>> btw doxy is also wrong in avformat.h.
> 
> But 1/sample_rate is the stream timebase in this case, so 1 frame is
> equivalent to 1 timebase unit.

no, 1 frame is long as how many samples are in your frame, being said,
one ac3 frame contains 1536 samples at 48khz, duration of one frame is
1536/48000, and assuming you have 50 frames, you have 76800 samples, and
duration is 76800 / 48000, not 50 / 48000.

> [...]
> Index: libavformat/Makefile
> ===================================================================
> --- libavformat/Makefile	(revision 8622)
> +++ libavformat/Makefile	(working copy)
> @@ -28,6 +28,7 @@
>  OBJS-$(CONFIG_AVI_MUXER)                 += avienc.o riff.o
>  OBJS-$(CONFIG_AVISYNTH)                  += avisynth.o
>  OBJS-$(CONFIG_AVS_DEMUXER)               += avs.o vocdec.o voc.o riff.o
> +OBJS-$(CONFIG_CAFF_DEMUXER)              += caffdec.o caff.o riff.o

need isom.o also.

> [...]
> -    }
> -    return 0;
> +    return ff_mov_read_esds(c->fc, &sc->esds);
>  }

ff_mov_read_esds(c->fc, &sc->esds);
return 0;

maybe ff_mov_read_esds should only take ByteIOContext.

> [...]
>
> +static int caff_read_kuki(AVFormatContext *s, int64_t size)
> +{
> +    ByteIOContext *pb = &s->pb;
> +    CaffContext *ctx = s->priv_data;
> +
> +    if(size < 0)
> +        return -1;
> +
> +    if(ctx->avctx->codec_id == CODEC_ID_AAC) {
> +        /* The magic cookie format for AAC is an mp4 esds atom.
> +           The lavc aac decoder requires the data from the codec specific
> +           description as extradata input. */
> +        MOV_esds_t esds;
> +        int strt, skip;
> +
> +        strt = url_ftell(pb);
> +        ff_mov_read_esds(s, &esds);

IMHO size should be passed to read_esds and it will skip data.

> +        skip = size - (url_ftell(pb) - strt);
> +        if(skip < 0 || !ctx->avctx->extradata ||
> +                ctx->avctx->codec_id != CODEC_ID_AAC) {
> +            av_log(s, AV_LOG_ERROR, "invalid AAC magic cookie\n");
> +            return AVERROR_INVALIDDATA;
> +        }
> +        url_fskip(pb, skip);
> +    } else {
> +        /* read all kuki chunk data into extradata */
> +        ctx->avctx->extradata = av_mallocz(size + FF_INPUT_BUFFER_PADDING_SIZE);
> +        if(ctx->avctx->extradata) {
> +            get_buffer(pb, ctx->avctx->extradata, size);
> +            ctx->avctx->extradata_size = size;

add a check like in mov_read_extradata to avoid overflow.

if((uint64_t)size > (1<<30))
   return -1;

> [...]
> +        if(ctx->bytes_per_packet == 0) {
> +            ctx->packet_table[i].bytes = ff_mov_mp4_read_descr_len(pb);
> +        } else {
> +            ctx->packet_table[i].bytes = ctx->bytes_per_packet;
> +        }
> +        if(ctx->frames_per_packet == 0) {
> +            ctx->packet_table[i].frames = ff_mov_mp4_read_descr_len(pb);
> +        } else {
> +            ctx->packet_table[i].frames = ctx->frames_per_packet;
> +        }

maybe ternary or without { would be easier to read but that's up to you.

> [...]
> +
> +            default:
> +                av_log(s, AV_LOG_WARNING, "skipping CAFF chunk: %08X\n", tag);

DEBUG no ?

> [...]
> +static int caff_read_seek(AVFormatContext *s, int stream_index,
> +                          int64_t timestamp, int flags)
> +{
> +    CaffContext *ctx = s->priv_data;
> +    int64_t fpos=0, bpos=0;
> +
> +    fpos = FFMAX(0, ctx->frame_cnt + timestamp);

timestamp will be in stream timebase. Are you sure that this works ?

> +    if(ctx->frames_per_packet > 0 && ctx->bytes_per_packet > 0) {
> +        /* calculate new byte position based on target frame position */
> +        bpos = ctx->bytes_per_packet * fpos / ctx->frames_per_packet;
> +        bpos = FFMIN(bpos, ctx->data_size);
> +        ctx->packet_cnt = bpos / ctx->bytes_per_packet;
> +        ctx->frame_cnt = ctx->frames_per_packet * ctx->packet_cnt;
> +    } else if(ctx->has_packet_table) {
> +        /* scan through packet table for target frame position */
> +        int64_t p = ctx->packet_cnt;
> +        if(timestamp < 0) {
> +            while(ctx->packet_table[p].fpos > fpos && p > 0)
> +                p--;

Negative timestamp ?

> +        } else {
> +            while(ctx->packet_table[p].fpos < fpos && p < ctx->num_packets)
> +                p++;
> +            p = FFMAX(p-1, 0);
> +        }
> +        ctx->packet_cnt = p;
> +        ctx->frame_cnt = ctx->packet_table[p].fpos;
> +        bpos = ctx->packet_table[p].bpos;

maybe it is simpler to use AVIndex when parsing header ?

libavformat version bump is missing also.

[...]

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