[FFmpeg-devel] [PATCH] oma demuxer

[Michael Niedermayer]

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


[...]

> #include "libavformat/avformat.h"

#include "avformat.h"


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


[...]
> const AVCodecTag codec_oma_tags[] = {

static const


>     { CODEC_ID_ATRAC3,  OMA_CODECID_ATRAC3 },
>     { CODEC_ID_ATRAC3P, OMA_CODECID_ATRAC3P },
> };
> 

> static int oma_read_header(AVFormatContext *s,
>                            AVFormatParameters *ap)
> {

>     uint16_t srate_tab[6] = {320,441,480,882,960,0};

static const


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


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

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.


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


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


[...]

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


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


[...]

>     if (st->codec->codec_id==CODEC_ID_ATRAC3) {

That can be merged with the switch above


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


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


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

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20080608/3272e53d/attachment.pgp>