[FFmpeg-devel] [PATCH] oma demuxer

[Michael Niedermayer]

On Sun, Jun 08, 2008 at 06:08:23PM +0200, Benjamin Larsson wrote:
> 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.


looks ok

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

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- 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/730bb711/attachment.pgp>