[FFmpeg-devel] [PATCH] R3D REDCODE demuxer

[Michael Niedermayer]

On Sat, Jan 17, 2009 at 05:42:10PM -0800, Baptiste Coudurier wrote:
> Hi Michael,
> 
> Michael Niedermayer wrote:
> > On Wed, Dec 17, 2008 at 12:05:25AM -0800, Baptiste Coudurier wrote:
> >> Hi,
> >>
> >> Here is the demuxer.
> >>
> >> Basically, chunks contains jp2 frames which can be decoded in some way
> >> by openjpeg.
> >>
> >> I'll add seeking as soon as code is in svn.
> > [...]
> >> +static int r3d_read_red1(AVFormatContext *s)
> >> +{
> >> +    AVStream *st = av_new_stream(s, 0);
> >> +    int tmp, tmp2, timebase;
> >> +
> >> +    if (!st)
> >> +        return -1;
> >> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> >> +    st->codec->codec_id = CODEC_ID_JPEG2000;
> >> +    tmp  = get_byte(s->pb); // major version
> >> +    tmp2 = get_byte(s->pb); // minor version
> >> +    dprintf(s, "version %d.%d\n", tmp, tmp2);
> >> +    get_be16(s->pb); // unknown
> > 
> >> +    timebase = get_be32(s->pb);
> >> +    dprintf(s, "timebase %d\n", timebase);
> >> +    st->time_base.num = 1;
> >> +    st->time_base.den = timebase;
> > 
> > av_set_pts_info()
> 
> Changed.
> 
> > also, i think the code would be more readable with an occasional empty
> > line :)
> 
> Yeah, I've gone too far :>
> 
> > [...]
> >> +static int r3d_read_header(AVFormatContext *s, AVFormatParameters *ap)
> >> +{
> >> +    R3DContext *r3d = s->priv_data;
> >> +    Atom atom;
> >> +    int ret;
> >> +
> >> +    if (read_atom(s->pb, &atom) < 0) {
> >> +        av_log(s, AV_LOG_ERROR, "error reading atom\n");
> >> +        return -1;
> >> +    }
> >> +    dprintf(s, "atom %d %.4s offset %#llx\n",
> >> +            atom.size, (char*)&atom.tag, atom.offset);
> >> +    if (atom.tag == MKTAG('R','E','D','1')) {
> >> +        if ((ret = r3d_read_red1(s)) < 0) {
> >> +            av_log(s, AV_LOG_ERROR, "error parsing 'red1' atom\n");
> >> +            return ret;
> >> +        }
> >> +    } else {
> >> +        av_log(s, AV_LOG_ERROR, "could not find 'red1' atom\n");
> >> +        return -1;
> >> +    }
> >> +
> >> +    s->data_offset = url_ftell(s->pb);
> >> +    dprintf(s, "data offset %#llx\n", s->data_offset);
> >> +    if (url_is_streamed(s->pb))
> >> +        return 0;
> >> +    // find REOB/REOF/REOS to load index
> >> +    url_fseek(s->pb, url_fsize(s->pb)-48-8, SEEK_SET);
> > 
> >> +    if (read_atom(s->pb, &atom) < 0) {
> >> +        av_log(s, AV_LOG_ERROR, "error reading end atom\n");
> >> +        return -1;
> >> +    }
> > 
> > if return 0 was ok before when url_is_streamed() then it should be ok
> > here too after seeking back, same for the other failure cases below
> 
> Right, changed.
> 
> >> +    dprintf(s, "atom %d %.4s offset %#llx\n",
> >> +            atom.size, (char*)&atom.tag, atom.offset);
> > 
> > These dprintfs() could be put in read_atom()
> 
> Good idea, done.
> 
> > [...]
> >> +static int r3d_read_reda(AVFormatContext *s, AVPacket *pkt, Atom *atom)
> >> +{
> >> +    int tmp, tmp2, samples, size;
> >> +    uint64_t pos = url_ftell(s->pb);
> >> +    unsigned dts;
> >> +
> >> +    if (s->nb_streams < 2)
> >> +        return -1;
> >> +    if (s->streams[1]->discard == AVDISCARD_ALL)
> >> +        return 0;
> >> +    dts = get_be32(s->pb);
> >> +    dprintf(s, "dts %d\n", dts);
> >> +    s->streams[1]->codec->sample_rate = get_be32(s->pb);
> >> +    samples = get_be32(s->pb);
> >> +    dprintf(s, "samples %d\n", samples);
> >> +    tmp = get_be32(s->pb);
> >> +    dprintf(s, "packet num %d\n", tmp);
> >> +    tmp = get_be16(s->pb); // unkown
> >> +    dprintf(s, "unknown %d\n", tmp);
> > 
> >> +    tmp  = get_byte(s->pb); // major version
> >> +    tmp2 = get_byte(s->pb); // minor version
> >> +    dprintf(s, "version %d.%d\n", tmp, tmp2);
> > 
> > id also suggest to either drop the dprintf() or write a
> > little macro that does get_* + dprintf() at once
> 
> Well this is an unspecified format and to macro it, I must deal with
> different types length, etc ...
> 
> I added seeking too.
> 
> Updated patch attached.
[...]
> +typedef struct {

> +    unsigned video_offsets_count;
> +    unsigned *video_offsets;

It may be simpler to use the avindex functions though then it would
eat more mem&cpu so its maybe a poor suggestion and the current code is
better, just wanted to mention it ...


> +    unsigned rdvo_offset;

> +    int timescale;

this contains the same value as AVStream.time_base thus it seems redundant


> +    AVRational frame_rate;

cant AVCodecContext.time_base be used ?


> +    int major, minor; ///< version

practically unused


> +} R3DContext;
[...]
> +static int r3d_read_red1(AVFormatContext *s)
> +{
> +    R3DContext *r3d = s->priv_data;
> +    AVStream *st = av_new_stream(s, 0);
> +    int tmp;
> +
> +    if (!st)
> +        return -1;
> +    st->codec->codec_type = CODEC_TYPE_VIDEO;
> +    st->codec->codec_id = CODEC_ID_JPEG2000;
> +
> +    r3d->major = get_byte(s->pb); // major version
> +    r3d->minor = get_byte(s->pb); // minor version
> +    dprintf(s, "version %d.%d\n", r3d->major, r3d->minor);
> +
> +    tmp = get_be16(s->pb); // unknown
> +    dprintf(s, "unknown %d\n", tmp);
> +
> +    r3d->timescale = get_be32(s->pb);
> +    dprintf(s, "timescale %d\n", r3d->timescale);
> +    av_set_pts_info(st, 32, 1, r3d->timescale);
> +
> +    tmp = get_be32(s->pb); // filenum
> +    dprintf(s, "filenum %d\n", tmp);
> +
> +    url_fskip(s->pb, 32); // unknown
> +
> +    st->codec->width  = get_be32(s->pb);
> +    st->codec->height = get_be32(s->pb);
> +    dprintf(s, "resolution %dx%d\n", st->codec->width, st->codec->height);
> +
> +    tmp = get_be16(s->pb); // unknown
> +    dprintf(s, "unknown %d\n", tmp);
> +
> +    r3d->frame_rate.den = get_be16(s->pb);
> +    r3d->frame_rate.num = get_be16(s->pb);
> +    dprintf(s, "frame rate %d/%d\n", r3d->frame_rate.num, r3d->frame_rate.den);
> +
> +    tmp = get_byte(s->pb); // audio channels
> +    dprintf(s, "audio channels %d\n", tmp);
> +    if (tmp > 0) {
> +        st = av_new_stream(s, 1);
> +        st->codec->codec_type = CODEC_TYPE_AUDIO;
> +        st->codec->codec_id = CODEC_ID_PCM_S32BE;
> +        st->codec->channels = tmp;
> +        av_set_pts_info(st, 32, 1, r3d->timescale);
> +    }
> +
> +    st->filename = av_mallocz(258);
> +    if (!st->filename)
> +        return AVERROR(ENOMEM);
> +    get_buffer(s->pb, st->filename, 257);
> +    dprintf(s, "filename %s\n", st->filename);

it might be cleaner to merge all the dprintf() and put them at the end of
each function


[...]
> +static int r3d_read_reos(AVFormatContext *s)
> +{
> +    R3DContext *r3d = s->priv_data;
> +    int tmp;
> +
> +    r3d->rdvo_offset = get_be32(s->pb);
> +    get_be32(s->pb); // rdvs offset
> +    get_be32(s->pb); // rdao offset
> +    get_be32(s->pb); // rdas offset
> +
> +    tmp = get_be32(s->pb);
> +    dprintf(s, "num video chunks %d\n", tmp);
> +
> +    tmp = get_be32(s->pb);
> +    dprintf(s, "num audio chunks %d\n", tmp);
> +
> +    url_fskip(s->pb, 6*4);
> +    return 0;
> +}

always returns 0 thus doesnt need to return anything yet


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

It is dangerous to be right in matters on which the established authorities
are wrong. -- Voltaire
-------------- 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/20090119/70418fa4/attachment.pgp>