Fw: Re: [Ffmpeg-devel] [PATCH] ported SGI decoder to the new API

Xiaohui Sun sunxiaohui
Mon Mar 26 04:28:30 CEST 2007


sunxiaohui wrote:
> ------------------------------------------------------------------------
> *????* Michael Niedermayer
> *?????* 2007-03-23 22:25:16
> *????* FFmpeg development discussions and patches
> *???*
> *???* Re: [Ffmpeg-devel] [PATCH] ported SGI decoder to the new API
> Hi
> On Fri, Mar 23, 2007 at 07:54:22PM +0800, sunxiaohui wrote:
> > Hi,
> > I have ported the SGI decoder to the new API.
> [...]
> > +#include "avcodec.h"
> > +#include "bytestream.h"
> > +
> > +/* sgi image file signature */
> > +#define SGI_MAGIC 474
> comments should be doxygen compatible
> [...]
> > +#define SGI_SINGLE_CHAN 2
> > +#define SGI_MULTI_CHAN 3
> unused
It is used to encode sgi images, which I will finish porting soon.
> > +
> > +typedef struct SgiContext {
> > + AVFrame picture;
> > +} SgiContext;
> > +
> > +typedef struct SGIInfo{
> > + short magic;
> > + char rle;
> > + char bytes_per_channel;
> > + unsigned short dimension;
> > + unsigned short xsize;
> > + unsigned short ysize;
> > + unsigned short zsize;
> > + unsigned short linesize;
> why are they not int?
they are 2-bytes parameter.
> > +} SGIInfo;
> why are there 2 structs (SGIInfo and SgiContext) and not 1
I will merge that.

> > +
> > +/* expand an rle row into a channel */
> > +static int expand_rle_row(uint8_t *buf, unsigned char *optr,
> > + int chan_offset, int pixelstride)
> > +{
> > + unsigned char pixel, count;
> > + int length = 0;
> > +
> > +#ifndef WORDS_BIGENDIAN
> > + /* rgba - > bgra for rgba32 on little endian cpus */
> > + if (pixelstride == 4 && chan_offset != 3) {
> > + chan_offset = 2 - chan_offset;
> > + }
> > +#endif
> > +
> > + optr += chan_offset;
> > +
> > + while (1) {
> > + pixel = bytestream_get_byte(&buf);
> > +
> > + if (!(count = (pixel & 0x7f))) {
> > + return length;
> > + }
> > + if (pixel & 0x80) {
> > + while (count--) {
> > + length++;
> > + optr += pixelstride;
> > + }
> > + } else {
> > + pixel = bytestream_get_byte(&buf);
> > +
> > + while (count--) {
> > + length++;
> > + optr += pixelstride;
> > + }
> > + }
> > + }
> what is this supposed to do?
I will check that.

> [...]
> > + tablen = ysize * zsize * sizeof(long);
> > + start_table = (unsigned long *)av_malloc(tablen);
> > + if (!bytestream_get_buffer(&cur_buf, (uint8_t*)start_table, tablen)) {
> unneeded memcpy() and unneeded casts
I will change.
> > + ret = AVERROR_IO;
> > + goto fail;
> > + }
> > +
> > + /* skip run length table */
> > + cur_buf += tablen;
> wrong indention
> > +
> > + for (z = 0; z < zsize; z++) {
> > + for (y = 0; y < ysize; y++) {
> > + dest_row = ptr + (ysize - 1 - y) * (xsize * zsize);
> whatever this is supposed to do the multiplication can overflow
> which considering this is a destination pointer to write is not good

then where should the decoded buffer be written to.
> > + start_offset = AV_RB32(&start_table[y + z * ysize]);
> > +
> > + if ((cur_buf - buf) != start_offset){
> > + cur_buf = buf + start_offset;
> > + }
> the check is useless
> [...]
> > +/* read an uncompressed sgi image */
> > +static int read_uncompressed_sgi(unsigned char* ptr,
> > + uint8_t *buf, int buf_size, SGIInfo* s_info)
> > +{
> > + int x, y, z, chan_offset, ret = 0;
> > + uint8_t *dest_row = ptr;
> > +
> > + /* skip header */
> > + buf += SGI_HEADER_SIZE - 12;
> > +
> > + for (z = 0; z < s_info- >zsize; z++) {
> > +
> > +#ifndef WORDS_BIGENDIAN
> > + /* rgba - > bgra for rgba32 on little endian cpus */
> > + if (s_info- >zsize == 4 && z != 3)
> > + chan_offset = 2 - z;
> > + else
> > +#endif
> > + chan_offset = z;
> > +
> > + for (y = s_info- >ysize - 1; y >= 0; y--) {
> > + dest_row = ptr + (y * s_info- >linesize);
> > + for (x = s_info- >xsize; x > 0; x--) {
> > + dest_row[chan_offset] = bytestream_get_byte(&buf);
> > + dest_row += s_info- >zsize;
> > + }
> > + }
> > + }
> isnt this just a memcpy() (with a loop for linesize)?

I think we can not just use memcpy, the destination format is not the
same with the source.
> [...]
> > + if(buf_size < 2){
> > + av_log(avctx, AV_LOG_ERROR, "buf size too small (%d)\n", buf_size);
> wrong indention
> > + return -1;
> > + }
> > +
> > + /* test for sgi magic */
> > + if (AV_RB16(buf) != SGI_MAGIC) {
> > + av_log(avctx, AV_LOG_ERROR, "bad magic number\n");
> indention should be 4 spaces in ffmpeg
> > + return -1;
> > + }
> > +
> > + /* skip magic */
> > + buf += 2;
> > + s_info.rle = bytestream_get_byte(&buf);
> > + s_info.bytes_per_channel = bytestream_get_byte(&buf);
> > + s_info.dimension = (unsigned short)bytestream_get_be16(&buf);
> > + s_info.xsize = (unsigned short) bytestream_get_be16(&buf);
> > + s_info.ysize = (unsigned short) bytestream_get_be16(&buf);
> > + s_info.zsize = (unsigned short) bytestream_get_be16(&buf);
> senseless casts
> bytes_per_channel, rle and dimension are not used outside this function so
> they dont need to be stored in the context
> xsize/ysize could be renamed to width/height
> > +
> > + if(s_info.zsize > 4096)
> > + s_info.zsize= 0;
> what is this good for, >4096 and 0 will both fail the checks below
> [...]
> > + s_info.linesize = p- >linesize[0];
> this is wrong, linesize doesnt have to fit into a unsigned short
> [...]
> > +static int sgi_init(AVCodecContext *avctx){
> > + SgiContext *s = avctx- >priv_data;
> > +
> > + avcodec_get_frame_defaults((AVFrame*)&s- >picture);
> > + avctx- >coded_frame= (AVFrame*)&s- >picture;
> senseless casts
> [...]
> -- 
> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> There will always be a question for which you do not know the correct
> awnser.
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
> iD8DBQFGA+ErYR7HhwQLD6sRAmV8AJ4kajsU5egVQ8J6pUN97msFAwelVQCfafie
> vWP1y5EAG/bsH4MjiILOnOU=
> =Q3T2
> -----END PGP SIGNATURE-----
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> http://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel






More information about the ffmpeg-devel mailing list