[FFmpeg-devel] [PATCH] oma demuxer

[Benjamin Larsson]

Michael Niedermayer wrote:
> On Mon, May 05, 2008 at 10:14:35PM +0200, Benjamin Larsson wrote:
>> Hi, here is a demuxer for the oma container. Sample file to test the
>> decoder can be found here:
>>
>> http://samples.mplayerhq.hu/oma/01-Untitled(1).oma
>>
> 
>> It works but spits out lots "Multiple frames in a packet" messages. What
>> has to be done to fix that issue.
> 
> Pass single packets instead of 4 at a time but thats obvious you even 
> explicitly multiply by 4.
> 

Fixed.

> [...]
>> +typedef struct {
> 
>> +    int   filesize;
> 
> unneeded
> 
> 
>> +    int   packetsize;
> 
> redundant
> 

What should be used instead then ?

> 
>> +    int   npackets;
> 
> unused
> 
> 
>> +    int   datapos;
> 
> unneeded
> 
> 
>> +} OMAContext;
>> +
> 
>> +typedef struct {
>> +    int   index;
>> +    int   bitrate;
>> +    int   framesize;
>> +    int   channels;
>> +    int   coding_mode;
>> +} ATRAC3ModeDesc;
>> +
>> +static const ATRAC3ModeDesc  atrac3_modes[4] = {
>> +    {0, 66000, 192, 2, 1},
>> +    {1, 94000, 272, 2, 1},
>> +    {2, 105000, 304, 2, 0},
>> +    {3, 132000, 384, 2, 0}
>> +};
> 
> this could be an anonymous struct
> int is not needed, smaller types are enough
> one field equals the index one is const
> 

Fixed.

> 
>> +
>> +static int is_ea3_tag_header(const uint8_t *buf)
>> +{
>> +    return (buf[0] == 'e' && buf[1] == 'a' && buf[2] == '3' && buf[3] == 3 && buf[4] == 0);
>> +}
> 
> strcmp, memcmp, whatever
> 

Fixed.

> 
>> +
> 
>> +static int16_t get_encryption_id(const uint8_t *buf)
>> +{
>> +    return (buf[0] << 8 | buf[1]);
>> +}
> 
> we have code to read 16bits
> 

Fixed.

> 
> [...]
>> +static int oma_read_header(AVFormatContext *s,
>> +                           AVFormatParameters *ap)
>> +{
>> +    int     filesize, ret, taglen, pos, codec_id, framesize, i, mode;
>> +    int16_t eid;
>> +    uint8_t buf[EA3_HEADER_SIZE];
>> +    const uint8_t *edata;
>> +    AVStream *st;
>> +    OMAContext *ctx = s->priv_data;
>> +
> 
>> +    filesize = url_fsize(s->pb);
>> +    if (filesize < 10)
>> +        return -1;
> 
> remove this
> 

Fixed.

> 
> [...]
>> +    st->codec->channels = atrac3_modes[mode].channels;
>> +    st->codec->sample_rate = 44100;
>> +    st->codec->bit_rate = atrac3_modes[mode].bitrate;
>> +    st->codec->block_align = atrac3_modes[mode].framesize;
> 
> vertical align 
> 

Fixed.

> 
>> +
>> +    /* fake the atrac3 extradata */
>> +    st->codec->extradata_size = 14;
>> +    edata = av_mallocz(14 + FF_INPUT_BUFFER_PADDING_SIZE);
>> +    if (!edata)
>> +        return AVERROR(ENOMEM);
>> +
>> +    st->codec->extradata = edata;
>> +    bytestream_put_le16(&edata, 1);
>> +    bytestream_put_le32(&edata, 4096);  // samples_per_channel
>> +    bytestream_put_le16(&edata, atrac3_modes[mode].coding_mode);
>> +    bytestream_put_le16(&edata, atrac3_modes[mode].coding_mode);
>> +    bytestream_put_le16(&edata, 1);
> 
>> +    bytestream_put_le16(&edata, 0);
> 
> redundant
> 

I think it is informative but no need to compile it thus commented out.

> 
> 
>> +
>> +    st->need_parsing = AVSTREAM_PARSE_FULL;
> 
> This does not work as there is no ATRAC3 parser
> 

Removed.

> 
> [...]
>> +static int oma_read_packet(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +    int ret, size, left;
>> +    OMAContext *ctx = s->priv_data;
>> +
>> +    size = ctx->packetsize * 4;
>> +    left = ctx->filesize - url_ftell(s->pb);
>> +    if (left < size)
>> +        size = left;
>> +
>> +    ret= av_get_packet(s->pb, pkt, size);
>> +
>> +    pkt->stream_index = 0;
>> +    if (ret <= 0) {
>> +        return AVERROR(EIO);
>> +    }
>> +    /* note: we need to modify the packet size here to handle the last
>> +       packet */
> 
>> +    pkt->size = ret;
> 
> ?
> 

Comment removed.


> 
>> +    return ret;
>> +}
>> +
> 
>> +#ifdef CONFIG_OMA_DEMUXER
>> +AVInputFormat oma_demuxer = {
>> +    "oma",
>> +    "Sony OpenMG audio",
>> +    sizeof(OMAContext),
>> +    oma_read_probe,
>> +    oma_read_header,
>> +    oma_read_packet,
>> +    NULL,
>> +    .flags= AVFMT_GENERIC_INDEX,
> 
>> +    .extensions = "oma,aa3", /* XXX: use probe */
> 
> senseles as a probe function exists.
> 

Does it hurt if they are left there ? Even if they aren't used to
trigger probes can't they be used for listing the extensions when
listing the formats (ffmpeg -formats)?

MvH
Benjamin Larsson
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oma.c
Type: text/x-csrc
Size: 7145 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080524/73cdcbd9/attachment.c>