[FFmpeg-devel] [PATCH] oma demuxer

[Michael Niedermayer]

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 ...


[...]
> > 
> > [...]
> >> +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


> 
> 
> > 
> >> +    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


[...]
> 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() ?


> }
> 
> 

> 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.


>     uint32_t    info;
> 
>     /* extract the atrac3 info string */
>     info = AV_RB24(&buf[33]);

declaration and initialization can be merged


> 
>     /* 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 ...


> 
>     /* 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


>         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


[...]
> 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()


[...]
>     /* 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.


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I wish the Xiph folks would stop pretending they've got something they
do not.  Somehow I fear this will remain a wish. -- M?ns Rullg?rd
-------------- 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/20080524/9c4fda87/attachment.pgp>