[FFmpeg-devel] [PATCH] oma demuxer

[Benjamin Larsson]

Michael Niedermayer wrote:
> On Sat, May 24, 2008 at 04:51:10AM +0200, Benjamin Larsson wrote:
>> Michael Niedermayer wrote:
> [...]
>>>> +    int   packetsize;
>>> redundant
>>>
>> What should be used instead then ?
> 
> you store it in:
> st->codec->block_align = atrac3_modes[mode].framesize;
> 
> so it can be used from there ...
> 

Ok, fixed but ugly IMO.

> 
> [...]
>>> [...]
>>>> +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.
> 
> i meant "pkt->size = ret;"
> not the comment
> 

Fixed.

> 
>>
>>>> +    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)?
> 
> hmm,i guess they dont hurt much ...
> a few bytes wasted, so if you want to keep them then keep them
> 

I'll add another item on the todo list.

> 
> [...]
>> static struct {
>>     uint32_t   bitrate;
>>     uint16_t   framesize;
>>     uint8_t    coding_mode;
>> }  atrac3_modes[4] = {
>>     {66000, 192, 1},
>>     {94000, 272, 1},
>>     {105000, 304, 0},
>>     {132000, 384, 0}
>> };
> 
> bitrate could be stored as uint8_t kbit
> 
>>
>> static int is_ea3_tag_header(const uint8_t *buf)
>> {
>>     return (!strncmp(buf, "ea3", 3) && buf[3] == 3 && buf[4] == 0);
> 
> memcmp() ?
> 

Fixed.

> 
>> }
>>
>>
> 
>> static void get_atrac3_info(const uint8_t *buf, int *frame_size, int *mode, int *sample_rate)
>> {
> 
> AVCodecContext could be passed as arg instead of these dozen pointers.
> 

Well there is no context left.

> 
>>     uint32_t    info;
>>
>>     /* extract the atrac3 info string */
>>     info = AV_RB24(&buf[33]);
> 
> declaration and initialization can be merged

Fixed.

> 
> 
>>     /* get framesize */
>>     /* soundUnit * 4 * nChannels */
>>     *frame_size = (info & 0x3FF) * 8;
> 
> the *8 could be moved to where block_align is inited, this would also allow
> uint8_t to be used i the table ...
> 

Fixed. But everything is now obfuscated, before it was possible to
understand the table by looking at it.

> 
>>     /* get coding mode */
>>     *mode = (info >> 17) & 1;
>>
>>     /* decode sample rate */
>>     switch ((info >> 13) & 7) {
> 
>>         case 0:
>>             *sample_rate = 32000;
>>             break;
>>         case 1:
>>             *sample_rate = 44100;
>>             break;
>>         case 2:
>>             *sample_rate = 48000;
>>             break;
> 
> ff_ac3_sample_rate_tab could be used here
> 

It's in the wrong order and 3 bits=6 elements can overflow the table. In
the future there might be other sample rates.

> 
>>         default:
>>             *sample_rate = 0;
>>     }
>> }
>>
>>
>> static int oma_read_probe(AVProbeData *p)
>> {
> 
>>     /* check file header */
>>     if (p->buf_size <= 10)
>>         return 0;
> 
> unneeded, see AVPROBE_PADDING_SIZE

Removed.

> 
> 
> [...]
>> static int oma_read_header(AVFormatContext *s,
>>                            AVFormatParameters *ap)
>> {
>>     int     ret, taglen, pos, codec_id, framesize, i, mode, jsflag, samplerate;
>>     int16_t eid;
>>     uint8_t buf[EA3_HEADER_SIZE];
>>     uint8_t *edata;
>>     AVStream *st;
>>     OMAContext *ctx = s->priv_data;
>>
>>     /* find the ea3 header */
>>     ret = get_buffer(s->pb, buf, 10);
> 
>>     if (ret != 10 || !is_ea3_tag_header(buf))
>>         return -1;
> 
> is_ea3_tag_header() is unneeded here, it was already checked in probe()
> 

Removed

> 
> [...]
>>     /* try to detect atrac3 mode using framesize */
>>     for (i = 0, mode = -1; i < 4 && mode == -1; i++) {
>>         if (atrac3_modes[i].framesize == framesize)
>>             mode = i;
>>     }
> 
> mode is unneeded, a break does as well.

Fixed.

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