[FFmpeg-devel] [PATCH] Core Audio Format demuxer (v5)
Michael Niedermayer
michaelni
Sat Sep 5 05:13:05 CEST 2009
On Wed, Aug 26, 2009 at 08:19:26PM +1000, Peter Ross wrote:
> On Sat, Aug 15, 2009 at 12:08:35PM +0200, Diego Biurrun wrote:
> > On Sat, Aug 15, 2009 at 05:35:42PM +1000, Peter Ross wrote:
> > >
> > > Revised patch enclosed.
> >
> > .. some nitpicks ..
>
> Ok. Revised patched enclosed.
[...]
> +typedef struct {
> + int bytes_per_packet; ///< bytes in a packet, or 0 if variable
> + int frames_per_packet; ///< frames in a packet, or 0 if variable
> +
> + int has_packet_table; ///< set if stream contains a packet table
does that ever differ from packet_table!=NULL ? if not
its redundant
[...]
> +/** Read audio description chunk */
> +static int read_desc_chunk(AVFormatContext *s)
> +{
> + ByteIOContext *pb = s->pb;
> + CaffContext *caf = s->priv_data;
> + AVStream *st;
> + int flags;
> +
> + /* new audio stream */
> + st = av_new_stream(s, 0);
> + if (!st)
> + return AVERROR_NOMEM;
> +
> + /* parse format description */
> + st->codec->codec_type = CODEC_TYPE_AUDIO;
> + st->codec->sample_rate = av_int2dbl(get_be64(pb));
> + st->codec->codec_tag = get_be32(pb);
> + flags = get_be32(pb);
> + caf->bytes_per_packet = get_be32(pb);
> + st->codec->block_align = caf->bytes_per_packet;
> + caf->frames_per_packet = get_be32(pb);
> + st->codec->channels = get_be32(pb);
I think we should have some sanity check for the number of channels
somewhere because we do multiply by the number of channels in various
places, its not directly related to your patch of course but a commit
adding such check at a central place would be welcome
> + st->codec->bits_per_coded_sample = get_be32(pb);
> +
> + /* calculate bit rate for constant size packets */
> + if (caf->frames_per_packet > 0 && caf->bytes_per_packet > 0) {
> + st->codec->bit_rate = st->codec->sample_rate * caf->bytes_per_packet * 8
> + / caf->frames_per_packet;
this can overflow
> + } else {
> + st->codec->bit_rate = 0;
> + }
> +
> + /* determine codec */
> + if (st->codec->codec_tag == MKBETAG('l','p','c','m'))
> + st->codec->codec_id = ff_mov_get_lpcm_codec_id(st->codec->bits_per_coded_sample, (flags ^ 0x2) | 0x4);
> + else
> + st->codec->codec_id = ff_codec_get_id(ff_codec_caf_tags, st->codec->codec_tag);
> + return 0;
> +}
> +
> +/** Read magic cookie chunk */
> +static int read_kuki_chunk(AVFormatContext *s, int64_t size)
> +{
> + ByteIOContext *pb = s->pb;
> + AVStream *st = s->streams[0];
> +
> + if (size < 0)
> + return -1;
> +
> + if (st->codec->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. */
> + int strt, skip;
> + MOVAtom atom;
> +
> + strt = url_ftell(pb);
> + ff_mov_read_esds(s, pb, atom);
> + skip = size - (url_ftell(pb) - strt);
> + if (skip < 0 || !st->codec->extradata ||
> + st->codec->codec_id != CODEC_ID_AAC) {
> + av_log(s, AV_LOG_ERROR, "invalid AAC magic cookie\n");
> + return AVERROR_INVALIDDATA;
> + }
> + url_fskip(pb, skip);
> + } else {
> + st->codec->extradata = av_mallocz(size + FF_INPUT_BUFFER_PADDING_SIZE);
> + if (!st->codec->extradata)
> + return AVERROR(ENOMEM);
> + get_buffer(pb, st->codec->extradata, size);
looks bad, size + FF_INPUT_BUFFER_PADDING_SIZE could overflow
leading to a too small allocated buffer
maybe its cought in some <0 check but it still looks risky ...
> + st->codec->extradata_size = size;
> + }
> +
> + return 0;
> +}
> +
> +/** Read packet table chunk */
> +static int read_pakt_chunk(AVFormatContext *s, int64_t size)
> +{
> + ByteIOContext *pb = s->pb;
> + CaffContext *caf = s->priv_data;
> + int64_t bcount = 0, fcount = 0, ccount;
> + int i;
> +
> + ccount = url_ftell(pb);
> +
> + caf->num_packets = get_be64(pb);
> + if (caf->num_packets < 0 || INT32_MAX / sizeof(PacketTableEntry) < caf->num_packets)
> + return AVERROR_INVALIDDATA;
> + caf->packet_table = av_mallocz(caf->num_packets * sizeof(PacketTableEntry));
> + if (!caf->packet_table)
> + return AVERROR_NOMEM;
> +
> + caf->has_packet_table = 1;
> + caf->num_frames = get_be64(pb); /* valid frames */
> + caf->num_frames += get_be32(pb); /* priming frames */
> + caf->num_frames += get_be32(pb); /* remainder frames */
> +
> + for (i = 0; i < caf->num_packets; i++) {
> + if (!caf->bytes_per_packet) {
> + caf->packet_table[i].bytes = ff_mp4_read_descr_len(pb);
> + } else {
> + caf->packet_table[i].bytes = caf->bytes_per_packet;
> + }
i tend to prefer
if(a)
else
over
if(!a)
else
but thats a minor thing
> + if (!caf->frames_per_packet) {
> + caf->packet_table[i].frames = ff_mp4_read_descr_len(pb);
> + } else {
> + caf->packet_table[i].frames = caf->frames_per_packet;
> + }
> + caf->packet_table[i].bpos = bcount;
> + caf->packet_table[i].fpos = fcount;
> + bcount += caf->packet_table[i].bytes;
> + fcount += caf->packet_table[i].frames;
considering that relation it might be possible to replace
frames by caf->packet_table[i].fpos - caf->packet_table[i-1].fpos
and similarly for bytes
if thats done maybe AVIndex could be used that also would then
provide us with a binary search for seeking which would be faster
for huge files
[...]
> +static int read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + ByteIOContext *pb = s->pb;
> + CaffContext *caf = s->priv_data;
> + AVStream *st;
> + uint32_t tag = 0;
> + int found_data, ret;
> + int64_t size;
> +
> + if (get_be32(pb) != MKBETAG('c','a','f','f')) {
> + av_log(s, AV_LOG_ERROR, "file type check failed\n");
> + return AVERROR_INVALIDDATA;
> + }
> + if (get_be16(pb) != 1) {
> + av_log(s, AV_LOG_ERROR, "file version check failed\n");
> + return AVERROR_INVALIDDATA;
> + }
these 2 should be redundant as the probe function already checked them
[...]
> +#define MAX_SIZE 4096
> +
> +static int read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + ByteIOContext *pb = s->pb;
> + CaffContext *caf = s->priv_data;
> + int res, pkt_size = 0, pkt_frames = 0;
> + int64_t left;
> +
> + if (url_feof(pb))
> + return AVERROR_IO;
> +
> + /* don't read past end of data chunk */
> + left = (caf->data_start + caf->data_size) - url_ftell(pb);
> + if (left <= 0)
> + return AVERROR_IO;
> +
> + pkt_frames = caf->frames_per_packet;
> + pkt_size = caf->bytes_per_packet;
> +
> + if (pkt_size > 0 && pkt_frames == 1) {
> + pkt_size = (MAX_SIZE / pkt_size) * pkt_size;
> + pkt_size = FFMIN(pkt_size, left);
> + pkt_frames = pkt_size / caf->bytes_per_packet;
> + } else if (caf->has_packet_table) {
> + if (caf->packet_cnt < caf->num_packets) {
> + pkt_size = caf->packet_table[caf->packet_cnt].bytes;
> + pkt_frames = caf->packet_table[caf->packet_cnt].frames;
> + } else {
> + return AVERROR_IO;
> + }
> + }
> +
> + if (pkt_size == 0 || pkt_frames == 0 || pkt_size > left)
> + return AVERROR_IO;
> +
> + res = av_get_packet(pb, pkt, pkt_size);
> + if (res < 0)
> + return res;
> +
> + pkt->size = res;
> + pkt->stream_index = 0;
i guess dts/pts could be set easily too
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090905/287842c0/attachment.pgp>
More information about the ffmpeg-devel
mailing list