[Ffmpeg-devel] [RFC] rtjpeg and NuppelVideo decoder/demuxer

[Michael Niedermayer]

Hi

On Tue, Feb 07, 2006 at 09:04:21PM +0100, Reimar D?ffinger wrote:
> Hi,
> the attached patch adds $subject to ffmpeg (well, actually it is a patch
> against MPlayer with libav* and untested on ffmpeg itself).
> The reason I'm posting this is that there is one thing I dislike but
> I do not know a good solution.
> The problem is, codec specific data, like the quantization tables for
> rtjpeg are "normal" packets, and at least the MPlayer demuxer passes
> them around as such.
> I very much prefer to have them as extradata, also because otherwise
> with MPlayer's approach there actually aren't any real keyframes - no
> single video packet contains _all_ the data to decode a full frame.
> Also, when the packet contains codec data, it does not encode any frame.
> Is there a way the codec should indicate this?
> _But_ if the decoder only supported the extradata approach it would not
> work with MPlayer's demuxer or files that were remuxed into AVI by it.
> Worse, it would be impossible to decode correctly files where the
> quantization tables change somewhere in between (I assume that extradata
> is not allowed to be changed?).
> So, the code currently does this: the decoder supports setting codec
> data both via extradata and in-stream, the demuxer puts the first codec
> data into extradata, all others (if any) in-stream.
> And one last thing concerning the demuxer: since nuv files contain
> timestamps given in milliseconds for each packet, I set r_frame_rate to
> the fps and used av_set_pts_info(vst, 32, 1, 1000);
> Is this the correct way to do it? It is a bit annoying, since this way
> MPlayer will not display the true framerate but 1000 fps instead...
> 
> Oh, and have a look if the patch to wav.c is acceptable (modulo better
> comment).

review finished, remaining comments below


[...]
> Index: libavcodec/bitstream.c
> ===================================================================
> RCS file: /cvsroot/ffmpeg/ffmpeg/libavcodec/bitstream.c,v
> retrieving revision 1.59
> diff -u -r1.59 bitstream.c
> --- libavcodec/bitstream.c	12 Jan 2006 22:43:14 -0000	1.59
> +++ libavcodec/bitstream.c	5 Feb 2006 19:33:07 -0000
> @@ -73,6 +73,12 @@
>      }
>  }
>  
> +void align_get_bits_to(GetBitContext *s, int align)
> +{
> +    int n= get_bits_count(s) % align;
> +    if(n) skip_bits(s, align - n);
> +}

are you crazy? ;) writing a function which does a slow % where a simple
skip_bits(s, get_bits_count(s)&1/2/4/...);
would be everything thats needed


[...]

> +#undef printf

10l ...


[...]
> +static int nuv_packet(AVFormatContext *s, AVPacket *pkt) {
> +    ByteIOContext *pb = &s->pb;
> +    uint8_t hdr[HDRSIZE];
> +    frametype_t frametype;
> +    int ret, size, done = 0;
> +    while (!url_feof(pb)) {
> +        ret = get_buffer(pb, hdr, HDRSIZE);
> +        if (ret <= 0)
> +            return ret;
> +        frametype = hdr[0];
> +        size = ((hdr[11] << 8 | hdr[10]) << 16) | (hdr[9] << 8 | hdr[8]);

LE_32() would be much more readable ...


> +        switch (frametype) {
> +            case NUV_VIDEO:
> +            case NUV_EXTRADATA:
> +                ret = av_new_packet(pkt, HDRSIZE + size);
> +                if (ret < 0)
> +                    return ret;
> +                pkt->pos = url_ftell(pb);
> +                pkt->pts = ((hdr[7] << 8 | hdr[6]) << 16) | (hdr[5] << 8 | hdr[4]);
> +                pkt->stream_index = 0;
> +                memcpy(pkt->data, hdr, HDRSIZE);
> +                ret = get_buffer(pb, pkt->data + HDRSIZE, size);

while this isnt exploitable its still a little risky (HDRSIZE + size) overflows,
av_new_packet allocates very little space, get_buffer() fails due to the
size being negative, if it where unsigned in get_buffer() it would be bad

IMHO size should be checked not only for security but also for error
resistance, you dont want to skip the whole file ..


[...]

-- 
Michael