[Ffmpeg-devel] LucasArts SANM (Smush v2) decoder

[Michael Niedermayer]

Hi

On Tue, Feb 27, 2007 at 01:23:36AM -0500, Cyril Zorin wrote:
> Ok, here's the new patch. I've fixed pretty much everything except as  
> noted below. Let me know if I missed anything.
> 

[...]

> 
> >>+static edge_t which_edge(int x, int y, int edge_size)
> >>+{
> >>+    edge_t edge;
> >>+    const int edge_max = edge_size - 1;
> >>+
> >>+    if (!y)
> >>+    {
> >>+        edge = bottom_edge;
> >>+    }
> >>+    else if (edge_max == y)
> >>+    {
> >>+        edge = top_edge;
> >>+    }
> >>+    else if (!x)
> >>+    {
> >>+        edge = left_edge;
> >>+    }
> >>+    else if (edge_max == x)
> >>+    {
> >>+        edge = right_edge;
> >>+    }
> >>+    else
> >>+    {
> >>+        edge = no_edge;
> >>+    }
> >>+
> >>+    return edge;
> >>+}
> >
> >if this function needs more 16byte then replace it by a table as this
> >can be done with 2 simple 8 byte tables indexed by i>>1 where
> >x= pxvector[i], y = pyvector[i];
> 
> Can you explain this a little more? I'm not sure how the tables are  
> constructed.

ok, first the x,y argument is always from glyph4/8_x/y, they have just 16
entries each so you can simply hardcode all awnsers with 2 16 entry tables
one for glyph4_x/yand one for glyph8
when you do you will IIRC notice that entry 2i and 2i+1 are always the
same so you can cut the table size in half ...


> 
> >>+    {
> >>+        dir = dir_right;
> >>+    }
> >>+
> >>+    return dir;
> >>+}
> >
> >
> >also the meaning of top_edge, bottom_edge and dir_down and dir_up  
> >doesnt
> >match top_edge + top_edge -> dir_down but its really toward that  
> >edge not
> >away if i understood the code correctly
> 
> It's trying to decide which way to fill the glyph given a line with  
> two points v0 and v1. It will fill the inside of the smaller region  
> of the glyph enclosed by the line.
> 
> >also this can be done with a 25byte table so if thats smaller  
> >please use
> >a table, the code above is totally unreadable anyway
> 
> Can you explain how to convert this to a lookup table?

well te function has 2 values as input each has 5 possible values so
you can build a lut[5][5] which contains all the awnsers for all the
possible inputs


[...]
> >>+static void fill_db(uint16_t* pbuf, int buf_size, uint16_t color)
> >>+{
> >>+    while (buf_size--)
> >>+    {
> >>+        *pbuf++ = color;
> >>+    }
> >>+}
> >
> >code duplication see msmpeg4_memsetw()
> 
> That function is static within msmpeg4.c. Maybe we should move it to  
> some utility header file?

yes thats what i was thinking, create a new file like memset.c memset.h
and add the 16bit memset into, other developers could if needed add 32bit 
and float memset equivalents to it later ...


> 
> >>+static void rotate_bufs(sanm_ctx_t* pctx, int rotate_code)
> >>+{
> >>+    if (1 == rotate_code)
> >>+    {
> >>+        swap(&pctx->pdb0, &pctx->pdb2);
> >>+    }
> >>+    else if (2 == rotate_code)
> >>+    {
> >>+        swap(&pctx->pdb1, &pctx->pdb2);
> >>+        swap(&pctx->pdb2, &pctx->pdb0);
> >>+    }
> >>+}
> >
> >if(2 == rotate_code)
> >    swap(&pctx->pdb1, &pctx->pdb2);
> >swap(&pctx->pdb2, &pctx->pdb0);
> 
> There is no guarantee that the rotate code is is always 1 or 2, so I  
> like to make it more explicit by pointing out both cases. We don't  
> know what to do if the rotate code is anything other than 1 or 2, so  
> I think it's better to leave it like this until we have the  
> aforementioned guarantee.

indeed, but then you should add a check where the value is read,
something like:

if(rotate != 1 && rotate != 2){
    av_log(context, AV_LOG_ERROR, "unknown rotate code, please contact developers and provide this file to them!\n");
}


[...]
> 
> >>+static void copy_block(uint16_t* pdest, uint16_t* psrc, int  
> >>block_size, int pitch)
> >>+{
> >>+    int y;
> >>+    for (y = 0; y != block_size; ++y, pdest += pitch, psrc += pitch)
> >>+    {
> >>+        memcpy(pdest, psrc, block_size * sizeof(pdest[0]));
> >>+    }
> >>+}
> >
> >code duplication see DSPContext.put_pixels_tab
> 
> The smush block sizes are 8x8, 4x4, and 2x2. Is this supported by  
> put_pixels_tab?

hmm i do think so 8x8 is [1][0] 4x4 [2][0] and 2x2 [3][0]
if any of them is NULL then just forget my comment and use your code


[...]
> >>+static void copy_output(sanm_ctx_t* pctx, frame_header_t* pheader)
> >>+{
> >>+    uint8_t* poutput = pctx->output.data[0];
> >>+    const uint8_t* pinput = (uint8_t*) pctx->pdb0;
> >>+
> >>+    int height = pctx->height;
> >>+    long inpitch = pctx->pitch * sizeof(pctx->pdb0[0]);
> >>+    long outpitch = pctx->output.linesize[0];
> >>+    while (height--)
> >>+    {
> >>+        memcpy(poutput, pinput, inpitch);
> >>+        pinput += inpitch;
> >>+        poutput += outpitch;
> >>+    }
> >
> >this is unacceptable, either use the buffers provided by get_buffer()
> >or return your buffer
> 
> I call "get_buffer" in decode_frame -- is that enough?

the idea is to not memcpy the frame but either return your internal
buffer and if possible use get_buffer() to allocate it, but note
get_buffer() does not gurantee that linesize == width*pixel_size
linesize can be larger, if this is a problem then dont use get_buffer()


> 
> >>+static int dispatch_codec(sanm_ctx_t* pctx, const uint8_t* pinput)
> >>+{
> >
> >why not remove this function and place the code into decode_frame?
> 
> I like giving each function one job to do. One function deal with  
> buffers/contexts/etc., another to figure out how to decode.

ok


[...]
> >>+        case MKBETAG('B', 'l', '1', '6'):
> >>+            if (size != av_get_packet(pbuf, ppacket, size))
> >>+            {
> >>+                return AVERROR_IO;
> >
> >memleak at EOF?
> 
> Sorry I don't see it. How particularly?

well if there is less then size bytes left av_get_packet will read less
you return failure so the user application would assume that nothing
has been allocated and wont free the smaller packet


> 
> >>+            ppacket->stream_index = pctx->vinfo.stream_index;
> >>+            ppacket->pts = pctx->vinfo.pts;
> >>+            ++pctx->vinfo.pts;
> >
> >do NOT set pts unless there is a pts in the packet (pts++ indicates  
> >that
> >you dont have a pts)
> 
> Hmm, my mistake. Can you explain what pts is actually used for? I  
> more or less copied this from another decoder and forgot to ask what  
> it was for =)

pts = presentation time stamp, its simply the time when the packet
after decoding it should be shown to the user, the libavformat
core will add +1 for you if you do nothing, at least it should
if everything like frame_rate if set correctly


> 
> >>+            done = 1;
> >>+            break;
> >
> >instead of the done=1; break stuff everywhere why not return 0; ?
> 
> Because the audio info chunk contains a bunch of headers that are  
> skipped, and I need to make sure that the stream seeks to the right  
> place once we got the info we're interested in. So instead of copying  
> the url_fseek call twice in each case label, I think it's cleaner to  
> give it an exit condition and then do the necessary "cleanup" (i.e.  
> url_fseek) right before return.

hmm there are 2 switch() with these done = 1 one could simply be changed
to return 0;

the other could do 

case MKBETAG('W', 'a', 'v', 'e'):
    painfo->freq = get_le32(pbuf);
    painfo->nchannels = get_le32(pbuf);
default:
    url_seek ...
    return something

case MKBETAG('B', 'l', '1', '6'):
    url_fseek(pbuf, size, SEEK_CUR);
    break;

}

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Democracy is the form of government in which you can choose your dictator
-------------- 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/20070228/2caf8c6a/attachment.pgp>