[FFmpeg-devel] [PATCH] oma demuxer

[Benjamin Larsson]

Michael Niedermayer wrote:
> On Sat, Jun 07, 2008 at 11:31:03PM +0200, Benjamin Larsson wrote:
>> Michael Niedermayer wrote:
> [...]
>> The code has some declarations that I think should be moved to
>> avformat.h. They are duplicated from riff.h.
> 
> no they do not belong in avformat.h, they are not part of the public API!
> if they should be in riff.h or not is debateable but they definitly do not
> belong in avformat.h
> 

My bad, it just felt stupid to include riff.h.

> 
> [...]
> 
>> #include "libavformat/avformat.h"
> 
> #include "avformat.h"
> 

Fixed.

> 
>> #include "libavutil/intreadwrite.h"
>> #include "raw.h"
>>
>> #define EA3_HEADER_SIZE 96
>>
> 
>> typedef struct AVCodecTag {
>>     int id;
>>     unsigned int tag;
>> } AVCodecTag;
>>
>> enum CodecID codec_get_id(const AVCodecTag *tags, unsigned int tag);
> 
> duplicate ...
> 

Removed.

> 
> [...]
>> const AVCodecTag codec_oma_tags[] = {
> 
> static const
> 
> 
>>     { CODEC_ID_ATRAC3,  OMA_CODECID_ATRAC3 },
>>     { CODEC_ID_ATRAC3P, OMA_CODECID_ATRAC3P },
>> };
>>

Fixed.

> 
>> static int oma_read_header(AVFormatContext *s,
>>                            AVFormatParameters *ap)
>> {
> 
>>     uint16_t srate_tab[6] = {320,441,480,882,960,0};
> 
> static const
> 

Fixed.

> 
> [...]
>>     /* 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;
>>     }
> 
> These should be AV_LOG_ERROR or FATAL not INFO
> 

Fixed.

> 
>>     /* Get generic codec parameters */
>>     info = AV_RB24(&buf[33]);
> 
> if info is generic codec parameters, why is it not named accordingly.
> codec_params or stream_params being an obvious choice ...
> And if you really are reading bits, maybe get_bits() would be cleaner ?

Renamed.

> 
> Please remove _ALL_ comments from the function body, see the linux kernel
> style guide for explanations why such comments are inapproriate except for
> really tricky things which I belive this simple demuxer does not qualify
> as.
> 

Removed.

> 
>>     /* Atrac3 specific: get coding mode, 1 for joint-stereo */
>>     jsflag = (info >> 17) & 1;
> 
> If its ATRAC3 specific why is it not under the appropriate if below?
> 

Moved.

> 
>>     channel_id = (info >> 10) & 7;
>>     samplerate = srate_tab[(info >> 13) & 7]*100;
>>
> 
>>     /* Check for invalid configurations */
>>     switch (buf[32]) {
>>         case OMA_CODECID_ATRAC3:
>>             framesize = (info & 0x3FF) * 8;
>>             if (samplerate != 44100) {
> 
>>                 av_log(s, AV_LOG_INFO, "Unsupported sample rate, send sample file to developers: %d\n", samplerate);
>>                 return -1;
> 
> AV_LOG_ERROR or FATAL see log.h
> If the demuxer fails and doesnt demux its not INFO though maybe it would be
> more appropriate not to fail ...
> 

Changed, we could always try to decode.

> 
> [...]
> 
>>     /* Set common parameters */
>>     st = av_new_stream(s, 0);
>>     if (!st)
>>         return AVERROR(ENOMEM);
>>
> 
> This creates a new stream it does not set common parameters
> 

Removed.

> 
>>     st->codec->codec_type  = CODEC_TYPE_AUDIO;
>>     st->codec->codec_id    = codec_get_id(codec_oma_tags, buf[32]);
>>     st->codec->channels    = channel_id;
>>     st->codec->sample_rate = samplerate;
>>     st->codec->bit_rate    = samplerate * framesize * 8 / 1024;
>>     st->codec->block_align = framesize;
> 
>>     st->codec->codec_tag   = 0;
> 
> buf[32] or more cleanly
> 
> st->codec->codec_tag   = buf[32]
> st->codec->codec_id    = codec_get_id(codec_oma_tags, st->codec->codec_tag);
> 

Fixed.

> 
> [...]
> 
>>     if (st->codec->codec_id==CODEC_ID_ATRAC3) {
> 
> That can be merged with the switch above
> 

Merged.

> 
> [...]
>> static int oma_read_packet(AVFormatContext *s, AVPacket *pkt)
>> {
> 
>>     int ret;
>>
>>     ret = av_get_packet(s->pb, pkt, s->streams[0]->codec->block_align);
> 
> declaration and initialization can be merged
> 

Merged.

> 
> [...]
>> static int oma_read_probe(AVProbeData *p)
>> {
>>     /* check file header */
>>     if (!memcmp(p->buf, (uint8_t[]){'e', 'a', '3', 3, 0},5))
>>         return AVPROBE_SCORE_MAX;
>>     else
>>         return 0;
>> }
> 
> its obvious that a *_probe() will likely check the file header, if you really
> think this has to be emphasized place the comment before the function please.
> Where it is currently it just makes the function hard to read.
> 

Removed.


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