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

[Nicholas T]

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?

For some reason, the test video I tried to attach last time didn't work. You
can download it from http://www.svatopluk.com/andux/files/vidtest4.zip.

Thanks for the detailed critique, Michael, it really helps me become a
better coder. A few things weren't just "fixed/done", but I'll include
everything so you can see I attended to it...

>> +//#define printf please_use_av_log
>> +//#define fprintf please_use_av_log
>unacceptable
No need to say so, it was obviously for debugging purposes.

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

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

> 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

> unneeded, someone really should remove these from all the codecs so they
dont constantly get copy and pasted around ...
done

> the code would be more readable if not everything where on the same line
fixed, I have a nice monitor, so I don't always notice

[about the defines]
defines are gone

> modulo is slow and doesnt belong into the innermost loop
fixed, converted to pointer to last line and x offset. this made going over
the frame buffer (being longer than the real frame width) easier as well.

> macros should be all UPPERCASED and i would prefer if no macros would be
> used to hide such trivial operations
fixed, defines gone

>> for(a = 0; a < 256; a++) { set_palette(a, a, a, a); }
> this is wrong
This was also sort of obviously debugging code. The palette is now set by
the file.
I didn't want to introduce new features at the risk of introducing new bugs,
and having
so many bugs I couldn't fix the first ones. This was actually rather
trivial.

> memset(scratch, buf++[0], rle_num_bytes);
thought I tried this and it didn't work, but oh well. It must have been
another bug. fixed.

> 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

> please put new codec_ids at the end of the list with the same codec type
done, and alphabetized in allcodecs.c

> i dont think thats true ...
forgot to fix that one...

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

> unused
fixed, was using for debugging

> the {} are superfluous also indention is wrong
I usually like {} b/c if you delete the statement afterwards you are messed
up. According to kwrite, which has pretty good format specifications, the
indentation isn't wrong, but I changed it anyway.

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

> the stream index of the first av_new_stream() call will always be 0, the
> second will always be 1, ...
> so decoder_stream_index is redundant
fixed

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

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

> what is this supposed to do, arent we already exactly at the next block
here?
There were quite a few examples that had seek calls, and stated in a comment
that the pointer might already be there. Fixed.

Nicholas

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