[FFmpeg-devel] [PATCH] oma demuxer

[Benjamin Larsson]

Michael Niedermayer wrote:
> 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.
> 

Removed some redundant comments.

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

Code inlined.

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

Fixed.

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

Fixed.

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

Added.


The code has some declarations that I think should be moved to
avformat.h. They are duplicated from riff.h.

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