[FFmpeg-devel] [PATCH] oma demuxer

[Michael Niedermayer]

On Tue, Jun 03, 2008 at 06:57:32PM +0200, Benjamin Larsson wrote:
[...]
> 
> 
> static int oma_read_header(AVFormatContext *s,
>                            AVFormatParameters *ap)
> {
>     int     ret, taglen, pos, codec_id, framesize, jsflag, samplerate, channel_id;
>     int16_t eid;
>     uint8_t buf[EA3_HEADER_SIZE];
>     uint8_t *edata;
>     AVStream *st;
> 
>     /* find the ea3 header */
>     ret = get_buffer(s->pb, buf, 10);
>     if (ret != 10)
>         return -1;
> 
>     /* get the length of the ea3 tag as syncsafe integer */
>     taglen = ((buf[6] & 0x7f) << 21) | ((buf[7] & 0x7f) << 14) | ((buf[8] & 0x7f) << 7) | (buf[9] & 0x7f);
> 
>     /* calculate file position of the "EA3" header */
>     pos = taglen + 10;
>     if (buf[5] & 0x10)
>         pos += 10;  /* skip the footer */
> 
>     /* read in the EA3 header */
>     url_fseek(s->pb, pos, SEEK_SET);
>     ret = get_buffer(s->pb, buf, EA3_HEADER_SIZE);
>     if (ret != EA3_HEADER_SIZE)
>         return -1;
> 
>     /* check the EA3 header */
>     if (memcmp(buf, (uint8_t[]){'E', 'A', '3'},3) || buf[4] != 0 || buf[5] != EA3_HEADER_SIZE) {
>         av_log(s, AV_LOG_INFO, "Couldn't find the EA3 header !\n");
>         return -1;
>     }
> 
>     /* check for encrypted content */
>     eid = AV_RB16(&buf[6]);
>     if (eid != -1 && eid != -128) {
>         av_log(s, AV_LOG_INFO, "Encrypted file! Eid: %d\n", eid);
>         return -1;
>     }

what are all the comments good for?
If you think its important for example to convey to he reader that pos is the
position of the EA3 header than IMHO it would be better to name the variable
ea3_header_pos or similarly instead of pos with a comment.

As is currently these comments make reading the code alot harder while
providing little if any information that is not obvious from the code.


> 
>     get_codec_parameters(buf, &framesize, &jsflag, &channel_id, &samplerate, &codec_id);

I think this function should be inlined or receive AVCodecContext as paramter
intead of these dozen pointers to redundant variables


> 
>     /* Check for invalid configurations */
>     switch (codec_id) {
>         case CODEC_ID_ATRAC3:
>             if (samplerate != 44100) {
>                 av_log(s, AV_LOG_INFO, "Unsupported sample rate, send sample file to developers: %d\n", samplerate);
>                 return -1;
>             }
>             break;
>         case CODEC_ID_ATRAC3P:
>             av_log(s, AV_LOG_INFO, "Unsupported codec ATRAC3+!\n");
>             return -1;
>             break;
>         default:
>             av_log(s, AV_LOG_INFO, "Unsupported codec %d!\n",codec_id);
>             return -1;
>             break;
>     }

the switch is duplicated


> 
>     /* Set common parameters */
>     st = av_new_stream(s, 0);
>     if (!st)
>         return AVERROR(ENOMEM);
> 
>     st->codec->codec_type  = CODEC_TYPE_AUDIO;
>     st->codec->codec_id    = codec_id;

codec_tag should be set too and see codec_get_id()


[...]
> AVInputFormat oma_demuxer = {
>     "oma",
>     "Sony OpenMG audio",
>     0,
>     oma_read_probe,
>     oma_read_header,
>     oma_read_packet,
>     NULL,
>     pcm_read_seek,
>     .flags= AVFMT_GENERIC_INDEX,
>     .extensions = "oma,aa3",
> };

teh codec_tag table should be set too


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

Old school: Use the lowest level language in which you can solve the problem
            conveniently.
New school: Use the highest level language in which the latest supercomputer
            can solve the problem without the user falling asleep waiting.
-------------- 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/20080603/1e332e2c/attachment.pgp>