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

Michael Niedermayer michaelni
Tue Mar 27 12:59:39 CEST 2007


Hi

On Tue, Mar 27, 2007 at 12:25:03AM -0700, Nicholas T wrote:
> New patch attached, pts didn't get fixed from Michael's second explanation
> (thanks for the info, though). Michael: could you be a bit more specific as
> to which file you want me to look at? I glanced at your NUT demuxer and it
> had pts all over the place--that isn't necessary for this, is it? I also saw
> that Mike Melanson's sega film works with the same pts demuxer code as my
> VID demuxer, so maybe the problem is in the decoder?

its pretty hard for anything in the decoder to affect the pts ...
the only thing which should be needed to get pts working is to set
the timebase and of course pts of every packet

have you tried to print the pts values you set in the packets to see if they
look ok?


[...]

> >>+// TODO: reduce redundant code
> >yes
> Should I make a .h file for the constants?

yes, libavcodec/bethsoftvideo.h or something like that might be a possible
choice


> 
> >trailing whitespace is forbidden in svn
> do you mean a newline with only spaces afterwards?

stuff like:
"abc     "
"      "

if we try to commit such, our fascist pre commit check script will shoot
us


> 
> >constants be they #defined or from a enum should be all UPPERCASE
> done, but they're a little bit less legible
> VIDEOOFFSETDIFFERENCEFRAME_BLOCK vs VideoOffsetDifferenceFrame_Block

well, multi word variable names can be seperated in various ways, one is
thisIsAVariable another is this_is_a_variable, first of course doesnt work
with all uppercased names

one easy way to reduce the length of the name is to use some common
abbreviations like P for predicted frame and I for intra frame
VIDEO_OFFSET_P_BLOCK

V_OFF_P_ID would be even shorter, choose whichever you like best ...


[...]
> >checking against overflows is needed yes but not afterwards, but rather
> before
> >the data is written
> sorta fixed, I'm ignoring the possibilities of overflow after a line wrap,
> b/c it's impossible with the typically supported frame widths

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 ...


[...]
> >please dont hide trivial things which are used once behind macrors
> I suppose I generally disagree with this: macros can help to explain things
> because they have names. But I guess for only one use, a comment is just as
> good.

static inline functions could be used too ...


[...]
> >this is wrong, you should set the proper timebase (and i suggest you dont
> >look in demuxers written by melanson ;) for figuring our how to do that)
> lol, thanks for the explaination afterwards. What are the wrap bits used for
> though?
> I'm still not getting PTS to work.

wrap bits are used for cases like
wrap_bits=8

254
255
0
1

this is common in mpeg* but uncommon in other containers so i dont think this
is where the problem lies


[...]
> >video decoding doesnt belong into the demuxer
> Bethsoft VID is a length-based format, so I have to read the entire frame to
> see if it terminates. I think this is correct.

yes indeed, Bethsoft VID seems to be a missdesigned container ...


> 
> >this is not a proper way to setup a AVPacket, theres av_new_packet() and
> >av_get_packet()
> fixed, though it's not an immensely useful function, replaces 2 lines of
> code with 1... I suppose the naming makes it more clear what's happening
> though.

well your code was wrong it would have been more then 2 lines otherwise


[...]

> +typedef struct VidDemuxContext

Vid is not correct as there are also audio packets


[...]
> +        int nframes;

written but never read


[...]

> +        int AUDIO_BLOCK_size;

unused


[...]

> +        /// index of a decoder instance; pass packet data
> +        int decoder_stream_index;

unused


[...]

> +    int npackets_read;

unused


[...]
> +/** internal function to read a palette block. Important precondition:
> + * pb must be seeked to character after block identifier. */
> +static int read_palette(VidDemuxContext *vid, ByteIOContext *pb, AVPacket *pkt)
> +{
> +    // read in a buffer for a VID_PALETTE_NUMCOLORS color palette with rgb channels
> +    unsigned char palette_buffer[VID_PALETTE_NUMCOLORS * 3];
> +    int a;
> +    uint32_t * palette;         // pointer to make int casting automatic
> +    if(get_buffer(pb, palette_buffer, VID_PALETTE_NUMCOLORS * 3) != VID_PALETTE_NUMCOLORS * 3)
> +        return AVERROR_IO;
> +    
> +    // additional alpha channel
> +    if(av_new_packet(pkt, VID_PALETTE_NUMCOLORS * 4 + 1)) { return AVERROR_NOMEM; }
> +    pkt->data[0] = PALETTE_BLOCK;
> +    palette = (uint32_t *)&pkt->data[1];
> +    
> +    for(a = 0; a < VID_PALETTE_NUMCOLORS; a++)
> +    {
> +        palette[a] = (0xff << 24 |                      // alpha = 1, if used
> +                palette_buffer[3 * a] << 18 |           // red * 4
> +                palette_buffer[3 * a + 1] << 10 |       // green * 4
> +                palette_buffer[3 * a + 2] << 2);        // blue * 4
> +    }

the convertion to native endian uint32_t palette entries belongs in the
decoder. as the output from the demuxer could be feeded to the avi/mov/...
muxer and the file then could be transfered from a little endian to a big
endian system demuxed and decoded there which would lead to the decoder
getting messed up palette packets


[...]
> +static int read_frame(VidDemuxContext *vid, ByteIOContext *pb, AVPacket *pkt, uint8_t block_type)
> +{
> +    // video buffer, ptr to start of array, don't mess around with dynamic allocation
> +    // stack allocation, so size can be relatively high. 2 if rle is very inefficient
> +    uint8_t vidbuf_start[vid->header.frame_width * vid->header.frame_height * 2];

the multiplication can overflow and cause the array to be smaller then
width*height


[...]
> +    // set the file position
> +    pkt->pos = url_ftell(pb);

this is wrong as the block_type byte is also part of the packet


> +    
> +    // set the block type for the decoder
> +    vidbuf++[0] = block_type;
> +    
> +    // get the video delay, and set the presentation time
> +    video_delay = get_le16(pb);
> +    // FIXME: pts isn't working
> +    vid->video.pts += (vid->header.bethsoft_global_delay + video_delay);

superfluous ()


> +    pkt->pts = vid->video.pts;
> +    
> +    // set the y offset if it exists (for the decoder)
> +    if(block_type == VIDEOOFFSETDIFFERENCEFRAME_BLOCK)
> +    {
> +        if(get_buffer(pb, vidbuf, 2) != 2) { return AVERROR_IO; }
> +        vidbuf += 2;
> +    }
> +    
> +    do
> +    {
> +        rle_num_bytes = get_byte(pb);

> +        vidbuf++[0] = (uint8_t)rle_num_bytes;

uneeded cast, also theres no sense in storing the rle decoded picture
we only need its length in the demuxer

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

I know you won't believe me, but the highest form of Human Excellence is
to question oneself and others. -- Socrates
-------------- 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/ee5a3fbe/attachment.pgp>



More information about the ffmpeg-devel mailing list