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

[Michael Niedermayer]

Hi

On Wed, Apr 04, 2007 at 11:39:25AM -0700, Nicholas T wrote:
[...]
> >On 4/3/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> >> if you dont have a lot of time now how do we know that you have a lot
> >> of time for the actual SOC?
> >Is this a rhetorical question? I will not be in school...if I get into
> >SOC I will not be doing anything else.

indeed ...


[...]
> >
> >> > okay, all  linesizes < width  now fail, as that's rather meaningless.
> >>
> >> its not meaningless, the image can be flipped and data[0] can point to
> >> the last line
> >How is the image flipped? Is that for negative linesize? 

yes


> >And for
> >linesize less than the width, am I supposed to truncate lines?

linesize less than width (that is abs(linesize)<width) is not allowed so
you can ignore it, it cannot happen


[...]
> >> significantly too complex for a rle decoder
> >Perhaps this is an illusion of my using comments and long variable
> >names? If you look at Konstantine's Autodesk RLE decoder (aasc.c),
> >it's 39 lines with a macro, whereas mine is 30 lines (not counting
> >comments, whitespace, and lines with only a brace). Nothing against
> >Konstantine...I'm sure that decoder has justification, but mine really
> >isn't that complex. What would you suggest to simplify it? It's a bit
> >of a pain dealing with the linesize and width.

ive been thinking about something like:

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;
}
end:


[...]
> >> breaks ABI
> >I should put bethsoftvid after thp? Thp was just added.

yes


> >
> >> > +    BVID_DemuxContext *vid = (BVID_DemuxContext *)s->priv_data;     
> >// permanent data outside of function
> >>
> >> useless cast
> >fixed, though it's in almost all of the other codecs...do you want me
> >to clean it up, along with empty functions (in another patch)? It
> >probably wouldn't be too difficult with a regex.

yes :)


> >
[...]
> >> why 16bit ?
> >video delays are 16 bit, 

yes, differencs between timestamps are 16bit this doesnt make the timestamps
16bit though


> this is what you told me when you were
> >explaining the timestamp, no? If the int16 overflows, the pts should
> >continue to increment, not actually going to 0, right?

[...]

> +/** frame decode handler */
> +static int bethsoftvid_decode_frame(AVCodecContext *avctx,

the comment is useless, it says nothing which isnt obvious from the
function name


[...]
> +    BethsoftvidContext * vid = (BethsoftvidContext *)avctx->priv_data;

useless cast

[...]
> +    if(vid->frame.linesize[0] < avctx->width) { return -1; }      // fail if the buffer is insufficient

breaks negative linesize


> +    line_start = vid->frame.data[0];

> +    frame_end = vid->frame.data[0] + (vid->frame.linesize[0] * avctx->height);

superfluous ()


[...]
> +        rle_num_bytes = buf++[0];

*buf++ is more commonly used, iam fine with buf++[0] too if you prefer


[...]
> +typedef struct BVID_DemuxContext
> +{
> +    int nframes;
> +        /** delay value between frames, added to individual frame delay.
> +     * custom units, which will be added to other custom units (~=16ms according
> +         * to free, unofficial documentation) */

indention doesnt look correct


[...]

> +    url_fseek(pb, 2, SEEK_CUR);

get_le16(pb);
is probably a better idea


[...]
> +#define BUFFER_PADDING_SIZE 1000
> +static int read_frame(BVID_DemuxContext *vid, ByteIOContext *pb, AVPacket *pkt,
> +                      uint8_t block_type, AVFormatContext *s, int npixels)
> +{
> +    // 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 = NULL;
> +    // current position in the buffer
> +    int vidbuf_nbytes = 0;
> +    int rle_num_bytes;
> +    int bytes_copied = 0;
> +    int position;
> +    size_t vidbuf_capacity = 0;
> +
> +    // initial allocation
> +    vidbuf_start = av_realloc(vidbuf_start, vidbuf_capacity += BUFFER_PADDING_SIZE);

this is av_malloc(1000) ...


[...]
> +    (&vidbuf_start[vidbuf_nbytes++])[0] = block_type;

this is vidbuf_start[vidbuf_nbytes++] = block_type;


[...]
> +        if(vidbuf_nbytes + BUFFER_PADDING_SIZE > vidbuf_capacity)
> +        {
> +            vidbuf_start = av_realloc(vidbuf_start, vidbuf_capacity += BUFFER_PADDING_SIZE);
> +            if(!vidbuf_start) { return AVERROR_NOMEM; }
> +        }

see av_fast_realloc()


[...]

> +#undef BUFFER_INCREMENT

useless


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/08fb5457/attachment.pgp>