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

Michael Niedermayer michaelni
Mon Mar 26 18:45:56 CEST 2007


Hi

On Mon, Mar 26, 2007 at 10:28:30AM +0800, Xiaohui Sun wrote:
[...]
> > [...]
> > > +#define SGI_SINGLE_CHAN 2
> > > +#define SGI_MULTI_CHAN 3
> > unused
> It is used to encode sgi images, which I will finish porting soon.

ok

also note encoder and decoder should be in seperate files


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

linesize is not a 2 byte parameter, also int is the "natural" variable size
of a architecture and thus tends to be faster than short, so when memory
space doesnt matter (8 byte per decoder isnt much ...) then int is generally
prefered


[...]
> > > + 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.

whereever you like, just not in unallocated memory


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

indeed ive missed that, the source is planar, still
random write with sequential read tends to be slower then sequential write
with random read

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/20070326/92e0f60b/attachment.pgp>



More information about the ffmpeg-devel mailing list