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

[Michael Niedermayer]

Hi

On Fri, Mar 30, 2007 at 01:45:11AM +0200, Marco Gerards wrote:
> Michael Niedermayer <michaelni at gmx.at> writes:
> 
> 
> 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.
> 
> [...]
> 
> > 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
> 
> Ok.  I have fixed this.
> 
> >
> > [...]
> >> 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
> 
> Fixed.  I have added the codec id to the end of the list.
> 
> 
> > [...]
> >> +  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?
> 
> Because the documentation doesn't mention this possibility.  And I
> doubt it will be used in reality.  
> Do you think this is important and

no, i just thought its odd that you skip any further video streams but
after actually looking at it i dont know how a second video stream would
be stored so its ok as it is


> are there samples which I can use to test this?

probably not


> 
> [...]
> 
> I also fixed all other things you commented on, see the new patch
> below.  The most mistakes I made were because I copied a lot from
> other demuxers.  Is there any good documentation available about how
> to do all this correctly instead of looking at other demuxers?

well ... i dont think so


[...]
> Index: libavcodec/mjpeg.c
> ===================================================================
> --- libavcodec/mjpeg.c	(revision 8550)
> +++ libavcodec/mjpeg.c	(working copy)
> @@ -2044,6 +2044,9 @@
>                          uint8_t x = *(src++);
>  
>                          *(dst++) = x;
> +                        if (avctx->codec_id != CODEC_ID_THP)
> +			{
> +
>                          if (x == 0xff)
>                          {
>                              while(src<buf_end && x == 0xff)
> @@ -2054,6 +2057,7 @@
>                              else if (x)
>                                  break;
>                          }
> +			}
>                      }

this looks much better, except the tabs, which we cannot commit to svn, even
if they would be removed in a second cosmetic only patch, our svn pre commit
check script is fairly heartless in this respect


[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h	(revision 8540)
> +++ libavcodec/avcodec.h	(working copy)
> @@ -255,6 +255,8 @@
>  
>      CODEC_ID_MPEG2TS= 0x20000, /* _FAKE_ codec to indicate a raw MPEG2 transport
>                           stream (only used by libavformat) */
> +
> +    CODEC_ID_THP
>  };

you can put CODEC_ID_THP at the end of the list of video IDs this doesnt
change the value of the following IDs as the first id after the video codecs
has a "hardcoded" value
in principle it would also work where you put it but its less nice looking


[...]
> +static int thp_read_header(AVFormatContext *s,
> +			   AVFormatParameters *ap)
> +{

more tabs, and there are even more below


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

No snowflake in an avalanche ever feels responsible. -- Voltaire
-------------- 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/929271bb/attachment.pgp>