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

Michael Niedermayer michaelni
Thu Apr 5 15:56:36 CEST 2007


Hi

On Wed, Apr 04, 2007 at 11:02:05PM -0700, Nicholas T wrote:
> Hi,
> 
>   Please answer my questions in the top part of the message - maybe
> you didn't see it, or forgot about it after reading inline
> comments...thanks

just repost any questions i missed


> 
> On 4/4/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >ive been thinking about something like:
> nice, though I don't think it would quite be functional. I implemented
> any simplifications though.
> 
> >remaining= width;
> >while(rle = *buf++){
> >    int len= rle&0x7F;
> >    while(len > remaining){
> >        if(rle < 0x80) bytestream_get_buffer(&buf, dst, remaining);
> >        else if(intra) memset(dst, *buf, remaining);
> you need to memcpy/memset up to the width, i.e. remaining - x, not
> just remaining. say you're in the middle of a line, you would copy
> into (linesize-width) instead of the correct pixels on the next line,
> though dst would be correct because of the two following lines. This
> is why there is the complication with current line, and xoffset.

remaining is what you call remaining - x


> 
> >        len -= remaining;
> >        dst += remaining + linesize - width;
> dst += linesize - width? remaining = width, so this is the same as +=
> linesize, which doesn't make sense.

remaining is not guarnteed to be = width


> 
> >        if(dst == picture_end)
> >            goto end;
> I like my checks, they aren't in sub-loops, and don't require gotos,
> despite slightly more arithmetic complexity.

i wouldnt call 
if(linesize_sign * (line_start + xoffset + length +
   ((xoffset + length) / avctx->width) * wrap_to_next_line - frame_end) > 0) { return -1; }

"slightly more"

also your divison-multiply based check is executed per byte while mine is
just executed once per line


> 
> >        remaining= width;
> you never set remaining elsewhere.

indeed, but the code was intended to show how it can be implemented with
significantly less complexity it wasnt really intended to work by just
being copy and pasted, anyway the following corrects remaining, its still
not tested and might contain other bugs ...

remaining= width;
while(rle = *buf++){
    int len= rle&0x7F;
    while(len > remaining){
        if(rle < 0x80) bytestream_get_buffer(&buf, dst, remaining);
        else if(intra) memset(dst, *buf, remaining);
        len -= remaining;
        dst += remaining + linesize - width;
        if(dst == picture_end)
            goto end;
        remaining= width;
    }
    if(rle < 0x80) bytestream_get_buffer(&buf, dst, len);
    else if(intra) memset(dst, *buf++, len);
    dst += len;
    remaining -= len;
}
end:


> 
> >    }
> >    if(rle < 0x80) bytestream_get_buffer(&buf, dst, len);
> >    else if(intra) memset(dst, *buf++, len);
> >    dst += len;
> >}
> >end:
> 
> >yes, differencs between timestamps are 16bit this doesnt make the 
> >timestamps
> >16bit though
> err, what do you want me to set it to? I'm not understanding,
> sorry...is the timestamp actually stored in an int16 now?

current behavior becomes undefined as soon as the pts you set is > 0xFFFF
as that violates your claim with ff_set_pts_info() that its just 16bit


> 
> >the comment is useless, it says nothing which isnt obvious from the
> >function name
> at the university they make us write comments for every function, even
> if the name is obvious, sorry for this junk.

cs teachers ...

normally code should be written so it is self explanatory that is by
choosing meaningfull function and variable names, comments make only
sense for what is not obvious from the names and structure or for
complicated code

int i;
for(i=0; i<number_of_people; i++){
    if(people[i].age > 30)
        old_people++;
}

vs. 

int i; /* variable used as people index */
for(i=0; i<number_of_people; i++){ // we iterate over all people
    // everyone who is older then 30
    if(people[i].age > 30) 
        // increase the number of old people
        old_people++;
}

its obvious which is more readable ...


[...]

> >*buf++ is more commonly used, iam fine with buf++[0] too if you prefer
> I can go with the standard, no problem. on the other hand,
> AV_RB32(&p->buf[0]) is pretty bad though...I accidentally copied it
> >from segafilm; in fact it's in 12 libavformat files.

someone should fix these ...


[...]

> +        palette[a] = AV_RB24(&palette_buffer[a * 3]) * 4;    // multiply all colors by 4

the mutiplication is obvious from the code, the reason why is less so a
comment like //bethsoftvid uses 3x6bit palette so we must multiply by 4 to get 3x8bit
would make slightly more sense though it could be argued thats obvious too


[...]

> +    // ignore cases where |linesize| < width, shouldn't happen
> +    line_start = vid->frame.data[0] + (vid->frame.linesize[0] < 0) * vid->frame.linesize[0] * avctx->height;
> +    frame_end = vid->frame.data[0] + (vid->frame.linesize[0] > 0) * vid->frame.linesize[0] * avctx->height;
> +    linesize_sign = 1 - 2 * (vid->frame.linesize[0] < 0);
> +    wrap_to_next_line = vid->frame.linesize[0] - avctx->width;    // this should work with negative linesize, goes to line before in memory

scary and i dont see what this is good for


[...]

> +        vidbuf_start = av_fast_realloc(vidbuf_start, &vidbuf_capacity, vidbuf_nbytes + BUFFER_PADDING_SIZE);
> +         if(!vidbuf_start) { return AVERROR_NOMEM; }

indention is off by 1 space


> +
> +        rle_num_bytes = get_byte(pb);
> +        vidbuf_start[vidbuf_nbytes++] = 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_start[vidbuf_nbytes++] = get_byte(pb); }
> +            bytes_copied += rle_num_bytes - 0x80;
> +        }
> +        else if(rle_num_bytes != 0)
> +        {
> +            if(get_buffer(pb, &vidbuf_start[vidbuf_nbytes], rle_num_bytes) != rle_num_bytes)
> +                return AVERROR_IO;
> +            vidbuf_nbytes += rle_num_bytes;
> +            bytes_copied += rle_num_bytes;
> +        }

bytes_copied += rle_num_bytes & 0x7F;


> +        if(bytes_copied == npixels)
> +        {
> +            // may contain a 0 byte even if read all pixels
> +            if(get_byte(pb)) { url_fseek(pb, -1, SEEK_CUR); }
> +            break;
> +        }
> +        if(bytes_copied > npixels) { return -1; }    // error
> +    } while(rle_num_bytes);

hmm, a 0 byte terminates the whole ...
do 0 bytes also occure in the middle of a packet? that is would a simple
copy all until the first 0 byte code work? this where alot simpler if it
would work ...


[...]
> +static int vid_read_packet(AVFormatContext *s,
> +                           AVPacket *pkt)
> +{
> +    BVID_DemuxContext *vid = s->priv_data;     // permanent data outside of function
> +    ByteIOContext *pb = &s->pb;                // io to file
> +    unsigned char block_type;                  // block type
> +    int audio_length;
> +    int ret_value;
> +
> +    if(vid->is_finished || url_feof(pb)) { return AVERROR_IO; }

is url_feof(pb) not enough?


[...]
> +            url_fseek(pb, url_ftell(pb) - 1, SEEK_SET);     // include block type

SEEK_CUR


[...]
> +        case AUDIO_BLOCK:
> +            av_log(s, AV_LOG_DEBUG, "audio block.\n");
> +            audio_length = get_le16(pb);
> +            ret_value = av_get_packet(pb, pkt, audio_length);
> +            pkt->stream_index = 1;
> +            return (ret_value != audio_length ? AVERROR_IO : ret_value);

if ret_value < audio_length && ret_value>0 then this will be a memleak


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I am the wisest man alive, for I know one thing, and that is that I know
nothing. -- 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/20070405/7a6ea2be/attachment.pgp>



More information about the ffmpeg-devel mailing list