[Ffmpeg-devel] Re: Bethsoft VID demuxer and decoder
Michael Niedermayer
michaelni
Tue Mar 27 01:42:34 CEST 2007
Hi
On Mon, Mar 26, 2007 at 02:19:53PM -0700, Nicholas T wrote:
> >Please provide a clean diff from the current svn.
>
> >lu
>
> Done. Note that this is preliminary; I still have other things to implement
> in the codec.
>
> If the files don't attach, I'll post them on http://ntung.com/index.
>
> Nicholas
>
> --
> http://ntung.com
> Index: libavutil/internal.h
> ===================================================================
> --- libavutil/internal.h (revision 8530)
> +++ libavutil/internal.h (working copy)
> @@ -252,8 +252,8 @@
> #define sprintf sprintf_is_forbidden_due_to_security_issues_use_snprintf
> #define strcat strcat_is_forbidden_due_to_security_issues_use_pstrcat
> #if !(defined(LIBAVFORMAT_BUILD) || defined(_FRAMEHOOK_H))
> -#define printf please_use_av_log
> -#define fprintf please_use_av_log
> +//#define printf please_use_av_log
> +//#define fprintf please_use_av_log
unacceptable
[...]
> +// TODO: reduce redundant code
yes
> +enum BethsoftVidBlockType
> +{
> + Palette_Block = 0x02,
> +
trailing whitespace is forbidden in svn
> + FirstAudio_Block = 0x7c,
> + Audio_Block = 0x7d,
> +
> + VideoFullFrame_Block = 0x03,
> + VideoNotOffsetDifferenceFrame_Block = 0x01,
> + /// difference frames with a y-offset
> + VideoOffsetDifferenceFrame_Block = 0x04,
> +
> + Finished_Block = 0x14,
> +};
constants be they #defined or from a enum should be all UPPERCASE
[...]
> + avctx->has_b_frames = 0;
unneeded, someone really should remove these from all the codecs so they
dont constantly get copy and pasted around ...
[...]
> +/** get the next frame pointer */
> +static uint8_t * get_next_frame_pointer(uint8_t * frame, int length, int x, int width, uint8_t * next_line)
> +{
> + if(x + length > width) { return next_line + length - (width - x); }
> + else { return frame + length; }
the code would be more readable if not everything where on the same line
[...]
> +#define set_palette(index, red, green, blue) ((uint32_t *) vid->frame.data[1])[index] = \
> + (0xff << 24 | red << 16 | green << 8 | blue);
> +#define check_overflow() if((frame_data - vid->frame.data[0]) > (vid->frame.linesize[0] * avctx->height)) \
> + { fprintf(stderr, "buffer overflow, %d bytes copied, %d bytes read.\n", (frame_data - vid->frame.data[0]), (buf - original_buffer)); exit(1); }
no demuxer or decoder may call exit(), abort() or similar in case of
invalid input
also this is nothing more then a simple if(curent_pos >= end) ...
the calculation of end in that doesnt have to be repeated in every iteration
also linesize can be negative
> +#define nbytes_written (frame_data - vid->frame.data[0])
unused
> +#define x_position ((frame_data - vid->frame.data[0]) % vid->frame.linesize[0])
modulo is slow and doesnt belong into the innermost loop
> +#define next_line (frame_data - x_position + vid->frame.linesize[0])
macros should be all UPPERCASED and i would prefer if no macros would be
used to hide such trivial operations
> + for(a = 0; a < 256; a++) { set_palette(a, a, a, a); }
this is wrong
> +
> + /// main code
> + do
> + {
> + rle_num_bytes = buf++[0];
> +
> + // if difference frame, skip rle_num_bytes, else set rle_num_bytes to the next byte
> + if(rle_num_bytes > 0x80)
> + {
> + rle_num_bytes -= 0x80;
> + if(block_type == VideoFullFrame_Block)
> + {
> + memset(scratch, buf[0], rle_num_bytes);
> + write_to_frame(frame_data, scratch, rle_num_bytes, x_position, avctx->width, next_line);
> + buf += 1;
memset(scratch, buf++[0], rle_num_bytes);
> + }
> + }
> +
> + // plain sequence of bytes, same for all video formats
> + else if(rle_num_bytes != 0)
> + {
> + write_to_frame(frame_data, buf, rle_num_bytes, x_position, avctx->width, next_line);
> + buf += rle_num_bytes;
> + }
> +
> + frame_data = get_next_frame_pointer(frame_data, rle_num_bytes, x_position, avctx->width, next_line);
> +
> + // fail on buffer overflows
> + check_overflow();
checking against overflows is needed yes but not afterwards, but rather before
the data is written
> + } while(rle_num_bytes);
> +
> + // FIXME: pts isn't working
> + sleep(1);
sleep() is not ok in a decoder but i think you knew that ...
[...]
> Index: libavcodec/avcodec.h
> ===================================================================
> --- libavcodec/avcodec.h (revision 8530)
> +++ libavcodec/avcodec.h (working copy)
> @@ -249,6 +249,8 @@
>
> CODEC_ID_MPEG2TS= 0x20000, /* _FAKE_ codec to indicate a raw MPEG2 transport
> stream (only used by libavformat) */
> +
> + CODEC_ID_BETHSOFTVID,
please put new codec_ids at the end of the list with the same codec type
(video here)
[...]
> Index: libavformat/bethsoftvid.c
> ===================================================================
> --- libavformat/bethsoftvid.c (revision 0)
> +++ libavformat/bethsoftvid.c (revision 0)
> @@ -0,0 +1,309 @@
[...]
> +
> +/**
> + * @file segafilm.c
> + * Sega FILM (.cpk) file demuxer
i dont think thats true ...
> + * by Mike Melanson (melanson at pcisys.net)
> + * For more information regarding the Sega FILM file format, visit:
> + * http://www.pcisys.net/~melanson/codecs/
> + */
> +
> +#include "avformat.h"
> +
> +#define VID_TAG MKTAG('V', 'I', 'D', 0)
please dont hide trivial things which are used once behind macrors
> +// 256 color palette
> +#define VID_PALETTE_NUMCOLORS 256
> +
> +#define FRAME_PTS_INC (90000 / 14)
unused
[...]
> +static int vid_probe(AVProbeData *p)
> +{
> + // little endian VID tag
> + if (p->buf_size < 4 || AV_RL32(&p->buf[0]) != VID_TAG)
> + { return 0; }
the {} are superfluous also indention is wrong
[...]
> + // header is constant length
> + vid->next_block = 15;
> +
> + /** tutorial material (?) - debug printing */
> + //fprintf(stdout, "\n\n=== parameters ===\nnumber of frames: %d\nframe width: %d\nframe height: %d\n",
> + //vid->header.nframes, vid->header.frame_width, vid->header.frame_height);
> +
> + // ffmpeg central code will use this; don't need to return or anything
> + // initialize the bethsoft codec
> + stream = av_new_stream(s, 0);
> + if (!stream) { return AVERROR_NOMEM; }
> + av_set_pts_info(stream, 33, 1, 90000);
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)
> + stream->codec->codec_type = CODEC_TYPE_VIDEO;
> + stream->codec->codec_id = CODEC_ID_BETHSOFTVID;
> + stream->codec->codec_tag = 0;
> + stream->codec->width = vid->header.frame_width;
> + stream->codec->height = vid->header.frame_height;
> + // FIXME: if this isn't set, codec has to reinitialize
> + stream->codec->pix_fmt = PIX_FMT_PAL8;
> + vid->video.decoder_stream_index = stream->index;
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
[...]
> +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];
> + // current position in the buffer
> + uint8_t * vidbuf = vidbuf_start;
> + int rle_num_bytes, vidbuf_nbytes;
> + int video_delay;
> + int bytes_copied = 0;
> +
> + // set the file position
> + pkt->pos = url_ftell(pb);
> +
> + // 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) * 160;
> + 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;
> +
> + // 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 == VideoFullFrame_Block) { vidbuf++[0] = (uint8_t)get_byte(pb); }
> + bytes_copied += rle_num_bytes - 0x80;
> + }
> + else if(rle_num_bytes != 0)
> + {
> + if(get_buffer(pb, vidbuf, rle_num_bytes) != rle_num_bytes)
> + { return AVERROR_IO; }
> + vidbuf += rle_num_bytes;
> + bytes_copied += rle_num_bytes;
> + }
> + if(bytes_copied == vid->header.npixels)
> + {
> + // may contain a 0 byte even if read all pixels
> + if(get_byte(pb)) { url_fseek(pb, url_ftell(pb) - 1, SEEK_SET); }
> + vidbuf++[0] = 0; // set a zero so the decoder doesn't have to mess with this
> + break;
> + }
> + assert(bytes_copied < vid->header.npixels);
> + } while(rle_num_bytes);
video decoding doesnt belong into the demuxer
> +
> + // copy packet data, and set some other parameters
> + vidbuf_nbytes = vidbuf - vidbuf_start; // doesn't undercount, increments to next byte above
> + pkt->data = av_malloc(vidbuf_nbytes * sizeof(char));
> + memcpy(pkt->data, vidbuf_start, vidbuf_nbytes);
> + pkt->size = vidbuf_nbytes;
> + pkt->stream_index = vid->video.decoder_stream_index;
this is not a proper way to setup a AVPacket, theres av_new_packet() and
av_get_packet()
[...]
> + // go to the next block
> + url_fseek(pb, vid->next_block, SEEK_SET);
what is this supposed to do, arent we already exactly at the next block here?
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- 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/9bbeb2e5/attachment.pgp>
More information about the ffmpeg-devel
mailing list