[Ffmpeg-devel] Re: Bethsoft VID demuxer and decoder
Michael Niedermayer
michaelni
Wed Mar 28 15:24:23 CEST 2007
Hi
On Tue, Mar 27, 2007 at 06:25:40PM -0700, Nicholas T wrote:
[...]
> >return -1
> done, and with the invalid starting block, the av_log is with ERROR
> instead of VERBOSE.
the code will not work with negative linesize
>
> >its not the job of the demuxer to decode the video frames
> >the demuxer doesnt has access to AVCodecContext.get/release_buffer() which
> >means it cant use direct rendering
> >decoding in the demuxer breaks stream copy
> >if the video is stored in another container which does store the packet
> >size then a decoder seperate from a demuxer is needed
> is there a misunderstanding here?
no you asked why to redo the thing in demuxer and decoder IIRC
> There is no decoding, but the whole
> frame does need to be read, and I was storing it as I read it, instead
> of storing the length and then reading it again. As you can see, for
> the case of RLE sequences, the demuxer does "vidbuf++[0] =
> get_byte(pb);", whereas the decoder does a memset.
ok
>
> >both these functions fail if a single run is over 3 lines
> >
> >also x + length occurs 5 times in the 2 functions, it likely would be
> >faster
> >not to redo this and hope the compiler would remove it
> >thats also one reason why i dont like to hide such trivial code in
> >functions
> >it hides such trivial optimization opertunities
> which it shouldn't be, because the width is 256 or 320. Do you suggest
> skipping the memset and just copying characters? This is what another
> codec did.
i dont know if memset() or simple char copying is faster, either way this
is unrelated to supporting runs over 3 lines
[...]
> >it should be more efficient to do the memsets directly into the destination
> >buffer
> but I can't use the same function twice...
?
>
> >the function does nothing and is thus unneeded
> it should have been doing something. the demuxer wasn't, so I deleted
> that. Mike Melanson: perhaps you could remove yours in idcin.c as
> well?
maybe someone could check all codecs and remove these empty init and close
functions ...
[...]
> >the demuxer should not modify the packet beyond the absolute neccesary
> I can't set a 0 so I don't have to do the check over again?
the deocder could be used together with another demuxer, for example
mplayer and xine use many of our decoders with their own demuxers and they
might not add such a 0 byte so the decoder shouldnt depend on that being
there
[...]
> >non constant static variables break thread safety
> I put this in BVID_DemuxContext; is that okay?
ok
[...]
> +/** write data to a frame with extra bytes on the end. */
> +static void write_to_frame(uint8_t * frame, uint8_t * buffer,
> + int length, int x, int width, uint8_t * next_line)
> +{
> + if(x + length > width)
> + {
> + // copy any bytes between the width and current position
> + memcpy(frame, buffer, width - x);
> + // copy on the new line anything that is left
> + memcpy(next_line, &buffer[width - x], length - (width - x));
> + }
> + else { memcpy(frame, buffer, length); }
> +}
> +
> +/** get the next frame pointer */
> +static void get_next_frame_pointer(uint8_t ** line_start, int length,
> + int * x, int width, uint8_t * next_line)
> +{
> + if(*x + length > width) { *x = length - (width - *x); *line_start = next_line; }
> + else { *x += length; } // didn't go to the next line, only increment x
> +}
x + length still occurs 5 times in the 2 functions
in addition it occurs 2 times outside now too
width - x occurs 6 times in the 2 functions and 2 times outside
and both functions get executed once in the innermost loop
please simplify your code
> +
> +static void set_palette(AVFrame * frame, uint8_t * palette_buffer)
> +{
> + uint32_t * palette = (uint32_t *)frame->data[1];
> + int a;
> + for(a = 0; a < VID_PALETTE_NUMCOLORS; a++)
> + {
> + palette[a] = (0xff << 24 | // alpha = 1, if used
> + palette_buffer[3 * a] << 18 | // red * 4
> + palette_buffer[3 * a + 1] << 10 | // green * 4
> + palette_buffer[3 * a + 2] << 2); // blue * 4
AV_RB24()
[...]
> +AVCodec bethsoftvid_decoder = {
> + .name = "bethsoftvid",
> + .type = CODEC_TYPE_VIDEO,
> + .id = CODEC_ID_BETHSOFTVID,
> + .priv_data_size = sizeof(BethsoftvidContext),
> + .init = bethsoftvid_decode_init,
> + .close = bethsoftvid_decode_end,
> + .decode = bethsoftvid_decode_frame,
> +};
indention is wrong
[...]
> + int frame_width;
> + int frame_height;
why duplicate AVCodecContext width/height?
[...]
> + struct {
> + /// same as video section, but individual delays are only added for the first frame.
> + int64_t pts;
> + } audio;
> +
> + struct {
> + /** video presentation time stamp.
> + * delay = 16 milliseconds * (global_delay + per_frame_delay) */
> + int64_t pts;
> + } video;
these 2 nested structs serve no real purpose
[...]
> Index: libavcodec/Makefile
> ===================================================================
> --- libavcodec/Makefile (revision 8531)
> +++ libavcodec/Makefile (working copy)
> @@ -178,6 +178,7 @@
> OBJS-$(CONFIG_ZLIB_ENCODER) += lcl.o
> OBJS-$(CONFIG_ZMBV_DECODER) += zmbv.o
> OBJS-$(CONFIG_ZMBV_ENCODER) += zmbvenc.o
> +OBJS-$(CONFIG_BETHSOFTVID_DECODER) += bethsoftvideo.o
please keep things in alphabetical order which where before your change
[...]
> +static int vid_read_header(AVFormatContext *s,
> + AVFormatParameters *ap)
> +{
> + BVID_DemuxContext *vid = (BVID_DemuxContext *)s->priv_data; // permanent data outside of function
> + ByteIOContext *pb = &s->pb; // io to file
> + unsigned char scratch[15]; // temporary buffer for file contents
> + AVStream *stream;
> +
> + /* load main header. Contents:
> + * bytes: 'V' 'I' 'D'
> + * int16s: always_512, nframes, width, height, delay, always_14
> + */
> + // assuming this is reading bytes into the scratch buffer (segafilm.c)
> + if (get_buffer(pb, scratch, 15) != 15)
> + return AVERROR_IO;
> + vid->header.nframes = AV_RL16(&scratch[5]);
> + vid->header.frame_width = AV_RL16(&scratch[7]);
> + vid->header.frame_height = AV_RL16(&scratch[9]);
> + vid->header.bethsoft_global_delay = AV_RL16(&scratch[11]);
> + vid->header.npixels = vid->header.frame_width * vid->header.frame_height;
> + vid->audio.pts = 0;
> +
> + // 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, 16, 1, 60); // 16 ms increments, i.e. 60 fps
isnt this supposed to be 185/sample_rate
[...]
> +static int read_frame(BVID_DemuxContext *vid, ByteIOContext *pb, AVPacket *pkt,
> + uint8_t block_type, AVFormatContext *s)
> +{
> + // 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];
as already said the multiplication can overflow
also stack space is not infinite but can be quite limited on some platforms
which makes it unsuitable for huge arrays
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
I count him braver who overcomes his desires than him who conquers his
enemies for the hardest victory is over self. -- Aristotle
-------------- 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/20070328/e13d1840/attachment.pgp>
More information about the ffmpeg-devel
mailing list