[Ffmpeg-devel] Re: Bethsoft VID demuxer and decoder

[Michael Niedermayer]

Hi

On Tue, Mar 27, 2007 at 11:07:26AM -0700, Nicholas T wrote:
[...]

> >trailing whitespace
> already fixed, please see previous message.

theres still trailing whitespace in the patch


> 
> >you missunderstand, no input valid or invalid may cause anything to be
> writen
> >to unallocated memory
> >
> >as one example why this is so bad lets assume the buffer is on the stack
> >each time a function is called the return address is put on the stack then
> >the code jumps to the code of the function and it reserves some space for
> >its local variables like our example buffer on the stack, now if we write
> >over the end of the buffer we may overwrite this return address.
> >but its not overwritten with any random value but rather with exactly what
> 
> >is stored in the file, lets assume that the new address will point into
> >some packet read from the file and that packet contains executable code
> >simply watching a video would turn your poor computer into someone elses
> >computer ;)
> >
> >also asserts are inappropriate to check input validity, an assert() simply
> >aborts the whole application if its not true. just think of printf()
> aborting
> >your editor (without it being able to safe your work) due to a failed
> assert()
> >similarely a video decoder may not abort the parent application because
> some
> >video is full of random damaged or even intentionally set bytes
> >
> >also asserts() are only enabled if debuging is so they are not appropriate
> 
> >to check for overflows as they simply arent always enabled ...
> I know why buffer overflows are bad...but what am I supposed to use instead
> of asserts?

return -1


> 
> >Vid is not correct as there are also audio packets
> The file format is called "VID", so I renamed this to "BVID", hopefully that
> will clear some things up

ok


[...]

> 
> >uneeded cast, also theres no sense in storing the rle decoded picture
> >we only need its length in the demuxer
> cast removed. what's the point of reading all of the file to get the length,
> then calling create packet, which
> will read it all over again?

its not the job of the demuxer to decode the video frames
the demuxer doesnt has access to AVCodecContext.get/release_buffer() which
means it cant use direct rendering
decoding in the demuxer breaks stream copy
if the video is stored in another container which does store the packet
size then a decoder seperate from a demuxer is needed
...


[...]

> +/** write data to a frame with extra bytes on the end. */
> +static void write_to_frame(uint8_t * frame, uint8_t * buffer,
> +                           int length, int x, int width, uint8_t * next_line)
> +{
> +    if(x + length > width)
> +    {
> +        // copy any bytes between the width and current position
> +        memcpy(frame, buffer, width - x);
> +        // copy on the new line anything that is left
> +        memcpy(next_line, &buffer[width - x], length - (width - x));
> +    }
> +    else { memcpy(frame, buffer, length); }
> +}
> +
> +/** get the next frame pointer */
> +static void get_next_frame_pointer(uint8_t ** line_start, int length,
> +                                        int * x, int width, uint8_t * next_line)
> +{
> +    if(*x + length > width) { *x = length - (width - *x); *line_start = next_line; }
> +    else { *x += length; }       // didn't go to the next line, only increment x
> +}

both these functions fail if a single run is over 3 lines

also x + length occurs 5 times in the 2 functions, it likely would be faster
not to redo this and hope the compiler would remove it
thats also one reason why i dont like to hide such trivial code in functions
it hides such trivial optimization opertunities


[...]
> +    else if(block_type == VIDEO_OFFSET_DIFFERENCE_FRAME_BLOCK)
> +    {
> +        line_start += vid->frame.linesize[0] * AV_RL16(buf);
> +        buf += 2;

bytestream_get_le16(&buf)


[...]
> +            if(block_type == VIDEO_FULL_FRAME_BLOCK)
> +            {
> +                assert(line_start + x + rle_num_bytes + (vid->linesize[0] - avctx->width) < frame_end);
> +                memset(scratch, buf++[0], rle_num_bytes);
> +                write_to_frame(&line_start[xoffset], scratch, rle_num_bytes,
> +                                xoffset, avctx->width, line_start + vid->frame.linesize[0]);

it should be more efficient to do the memsets directly into the destination
buffer


[...]
> +/** clean up resources */
> +static int bethsoftvid_decode_end(AVCodecContext *avctx)
> +{
> +    av_log(avctx, AV_LOG_VERBOSE, "[bethsoftvid video decoder] closing\n");
> +    return 0;
> +}

the function does nothing and is thus unneeded


[...]
> +    struct {
> +        int nframes;
> +        int frame_width;
> +        int frame_height;

> +        int npixels; // width * height

comment is not doxygen compatible


[...]
> +    struct {
> +        /** video presentation time stamp.
> +         * delay = 16 milliseconds * (global_delay + per_frame_delay) */
> +        /// video presentation time stamp

duplicate comment

[...]
> +    do
> +    {
> +        rle_num_bytes = get_byte(pb);
> +        vidbuf++[0] = rle_num_bytes;
> +
> +        // special codes for RLE: is an rle sequence (first case), is a plain sequence, 0 for stop
> +        if(rle_num_bytes > 0x80)
> +        {
> +            if(block_type == VIDEO_FULL_FRAME_BLOCK) { vidbuf++[0] = (uint8_t)get_byte(pb); }

the cast is unneeded


> +            bytes_copied += rle_num_bytes - 0x80;
> +        }
> +        else if(rle_num_bytes != 0)
> +        {
> +            if(get_buffer(pb, vidbuf, rle_num_bytes) != rle_num_bytes)
> +            { return AVERROR_IO; }
> +            vidbuf += rle_num_bytes;
> +            bytes_copied += rle_num_bytes;
> +        }
> +        if(bytes_copied == vid->header.npixels)
> +        {
> +            // may contain a 0 byte even if read all pixels
> +            if(get_byte(pb)) { url_fseek(pb, url_ftell(pb) - 1, SEEK_SET); }
> +            vidbuf++[0] = 0;        // set a zero so the decoder doesn't have to mess with this

the demuxer should not modify the packet beyond the absolute neccesary


> +            break;
> +        }
> +        assert(bytes_copied < vid->header.npixels);
> +    } while(rle_num_bytes);
> +
> +    // copy packet data, and set some other parameters
> +    vidbuf_nbytes = vidbuf - vidbuf_start; // doesn't undercount, increments to next byte above
> +    if(av_new_packet(pkt, vidbuf_nbytes)) { return AVERROR_NOMEM; }

calling av_new_packet(pkt) after setting pkt->pts of course cannot work


[...]
> +    static int is_finished;                         // if terminating block is reached, don't need to read junk afterwards

non constant static variables break thread safety


[...]
> +        case FIRST_AUDIO_BLOCK: av_log(s, AV_LOG_VERBOSE, "first audio block.\n"); exit(0);
> +        case AUDIO_BLOCK: av_log(s, AV_LOG_VERBOSE, "audio block.\n"); exit(0);

calling exit() in a demuxer is not ok


[...]
> +static int vid_read_close(AVFormatContext *s)
> +{
> +    av_log(s, AV_LOG_VERBOSE, "[bethsoftvid demuxer] closing\n");
> +    return 0;
> +}

this function does nothing so its unneeded

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

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- 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/20070327/66bf3bbf/attachment.pgp>