[Ffmpeg-devel] [PATCH] THP Demuxer (Summer of Code qualification task)

[Michael Niedermayer]

Hi

On Thu, Mar 29, 2007 at 11:33:38PM +0200, Marco Gerards wrote:
> Hi,
> 
> Here is a patch (sent inline) to add a THP demuxer to ffmpeg.  I've
> also changed the MPJEG decoder so it can play THP movies.  This is a
> qualification task for Google Summer of Code 2007.
> 
> It perfectly plays back the samples that can be found on the ffmpeg
> website.  Unfortunately these samples come without audio.  Because of
> this I haven't implemented audio support yet.  I hope someone can send
> me a sample that includes audio.  In that case I will implement this
> as well.
> 
> If I can do something to improve my code or to add something that is
> missing, please tell me.

[...]
> @@ -2044,17 +2045,20 @@
>                          uint8_t x = *(src++);
>  
>                          *(dst++) = x;
> -                        if (x == 0xff)
> -                        {
> -                            while(src<buf_end && x == 0xff)
> -                                x = *(src++);
> +                        if (avctx->codec_id != CODEC_ID_THP)
> +                        {
> +                            if (x == 0xff)
> +                            {
> +                                while(src<buf_end && x == 0xff)
> +                                    x = *(src++);
>  
> -                            if (x >= 0xd0 && x <= 0xd7)
> -                                *(dst++) = x;
> -                            else if (x)
> -                                break;
> +                                if (x >= 0xd0 && x <= 0xd7)
> +                                    *(dst++) = x;
> +                                else if (x)
> +                                    break;
> +                            }
>                          }
> -                    }
> +                    }


tabs are forbidden in svn
additionally cosmetic changes and functional changes must be in seperate
patches, that is dont change whitespace or indention just add the 
if (avctx->codec_id != CODEC_ID_THP){ ... } in the first patch
and fix the indention (and only the indention) in a second patch
this makes reviewing patches (and commits on svnlog) much easier


[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h        (revision 8540)
> +++ libavcodec/avcodec.h        (working copy)
> @@ -64,6 +64,7 @@
>      CODEC_ID_RV20,
>      CODEC_ID_MJPEG,
>      CODEC_ID_MJPEGB,
> +    CODEC_ID_THP,
>      CODEC_ID_LJPEG,

read the comment at the top of this CODEC_ID list!
you cannot add new codec_ids at random places this breaks the ABI

[...]
> +struct ThpDemuxContext {
> +    int              version;
> +    int              first_frame;
> +    int              first_framesz;
> +    int              last_frame;
> +    int              compoff;
> +    int              framecnt;
> +    double           fps;
> +    int              frame;
> +    int              next_frame;
> +    int              next_framesz;
> +    int              video_stream_index;
> +    int              compcount;
> +    unsigned char    components[16];
> +    AVStream*        vst;
> +};
> +typedef struct ThpDemuxContext ThpDemuxContext;

the struct and typedef can be combined


> +
> +
> +static int thp_probe(AVProbeData *p)
> +{
> +    /* check file header */
> +    if (p->buf_size < 4)
> +        return 0;
> +    if (p->buf[0] == 'T' && p->buf[1] == 'H' &&
> +        p->buf[2] == 'P' && p->buf[3] == '\0')
> +        return AVPROBE_SCORE_MAX;
> +    else
> +        return 0;
> +}

this can be simplified with MKTAG() and AV_RL32()


> +
> +/* thp input */
> +static int thp_read_header(AVFormatContext *s,
> +                           AVFormatParameters *ap)

comment isnt doxygen compatible


> +{
> +  ThpDemuxContext *thp = s->priv_data;
> +  AVStream *st;
> +  ByteIOContext *pb = &s->pb;
> +  int i;
> +
> +  /* Read the file header.  */
> +
> +  get_be32(pb); /* Skip Magic.  */
> +  thp->version = get_be32(pb);
> +
> +  get_be32(pb); /* Max buf size.  */
> +  get_be32(pb); /* Max samples.  */
> +
> +  thp->fps = av_int2flt(get_be32(pb));
> +  thp->framecnt = get_be32(pb);
> +  thp->first_framesz = get_be32(pb);
> +  get_be32(pb); /* Data size.  */

these would be slightly more readable if they where nicely aligned like

                      get_be32(pb);     /* Skip Magic.  */
thp->version        = get_be32(pb);
                      get_be32(pb);     /* Max buf size.  */
                      get_be32(pb);     /* Max samples.  */
thp->fps            = av_int2flt(get_be32(pb));
thp->framecnt       = get_be32(pb);
thp->first_framesz  = get_be32(pb);
                      get_be32(pb);     /* Data size.  */

of course there are millions of other ways to align this if you dont like
my example above


[...]
> +  for (i = 0; i < thp->compcount; i++) {
> +      if (thp->components[i] == 0) {

> +          if (thp->vst != 0)
> +             break;

why do you discard a second video stream?


[...]

> +          av_set_pts_info(st, 64, 1000, 1000 * thp->fps);

try av_d2q()


[...]
> +    if (av_new_packet(pkt, size))
> +       return AVERROR_IO;
> +
> +    ret = get_buffer(pb, pkt->data, size);

see av_get_packet()


[...]
> +static int thp_read_close(AVFormatContext *s)
> +{
> +    return 0;
> +}
> +

this function is unneeded just set the pointer to NULL


> +#ifdef CONFIG_THP_DEMUXER
> +AVInputFormat thp_demuxer = {
> +    "tph",
> +    "TPH",
> +    sizeof(ThpDemuxContext),
> +    thp_probe,
> +    thp_read_header,
> +    thp_read_packet,
> +    thp_read_close
> +};
> +#endif

the ifdef is unneeded as the whole file wont be compiled if its not true

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070330/8710de0f/attachment.pgp>