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

[Nicholas T]

Hi,

New questions / comments:
* fixed: no return after palette block in decoder, was dependent on
uninitialized memory being zero
* I removed this code, but if it does turn on some optimization, I can
put it back
BethsoftvidContext * vid = avctx->priv_data;
vid->frame.reference = 1;
    vid->frame.buffer_hints = FF_BUFFER_HINTS_VALID |
FF_BUFFER_HINTS_PRESERVE | FF_BUFFER_HINTS_REUSABLE;
    vid->frame.data[0] = NULL;
* removed audio pts, appears to be unneeded. Audio delay is calculated
by soundblaster dac as described on specification page, appears to be
working for dagvid.
* I think this is close to being a usable patch :) If anyone could
find some skynet


On 4/5/07, Michael Niedermayer <michaelni at gmx.at> wrote:
> just repost any questions i missed

I noticed that when I click the window in ffplay, it says "error
while seeking." This happens with other sample files as well...with a
segafilm sample file, like mine, it freezes on the current frame; with
a wmv sample file, it usually keeps playing, but sometimes freezes
near the end. Is it supposed to pause the video? the wmv file did
successfully seek to the beginning when I pressed the left arrow key.

> > 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
okay, remaining on each line is fine, I like that...though it doesn't
simplify everything, but at least there was only 1 place where I had
to add an instruction...it is definitely better. Thanks, good idea.

> remaining is not guarnteed to be = width
okay, you fixed that...

> 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
that checks to make sure it doesn't run off the buffer, I don't think
your code does that. By the time you add these features, it will
probably be comparable length. Suppose some other demuxer feeds it bad
data and doesn't do that check (mine does)...


> indeed, but the code was intended to show how it can be implemented with
> significantly less complexity
It was helpful, and I hope I have reduced enough.

> 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
right, because I am adding increments to an int32...fixed.


> > +    // 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
sorry, the start and the end were accidental mis-compensations for
negative linesize. The linesize_sign should be okay though; if it is
writing towards lower memory addresses, it should check that the
current position is greater than the end, not less than the end.

> indention is off by 1 space
fixed, sorry.

> 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 ...
no, because any data following a RLE num_bytes could be zero (zero
index in the palette), or any non-RLE sequence could contain a zero.
If only they used 2 bits at the beginning of the video packet to store
the length...

> is url_feof(pb) not enough?
no, the terminating block could occur before; on one sample I had to
read 1 byte, and I think this is more efficient than doing
while(url_feof(pb)) { get_byte(pb); }...that could waste considerable
time.

> SEEK_CUR
sorry, guess I missed that one...I will start reading over my files
more carefully.

> > +        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
true, fixed. Also in (just to list a few, and I am aware it's okay if
ret_value<=0, as av_get_packet cleans those up)
yuv4mpeg.c:369
mtv.c:158
electronicarts.c:239
idroq.c:227
ipmovie.c:145

Nicholas

-- 
http://ntung.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: bethsoft.diff
Type: text/x-patch
Size: 18288 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070406/114f3db1/attachment.bin>