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

[Cyril Zorin]

Ok, here's the new patch. I've fixed pretty much everything except as  
noted below. Let me know if I missed anything.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smush.patch
Type: application/octet-stream
Size: 33080 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070227/3c8f7a32/attachment.obj>
-------------- next part --------------

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

>> +    {
>> +        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?

> for(i=0; i<3; i++)
>     pctx->pdb[i]= av_mallocz(pctx->buf_size);
>
> and after that change this function is so trivial that it should be
> removed and the for loop be placed at the one and onle spot where this
> is called

Replaced with loop, except that I'd still like to keep the function.  
It makes things more clear.

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

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

>> +static void codec0(sanm_ctx_t* pctx, const uint8_t* pinput)
>> +{
>> +    uint16_t* pdb0 = pctx->pdb0;
>> +
>> +    int x, y;
>> +    for (y = 0; y != pctx->height; ++y)
>> +    {
>> +        for (x = 0; x != pctx->width; ++x)
>> +        {
>> +            *point_at(pdb0, x, y, pctx->pitch) = LE_16(pinput);  
>> pinput += 2;
>> +        }
>> +    }
>> +}
>> +
>> +static void codec6(sanm_ctx_t* pctx, const uint8_t* pinput)
>> +{
>> +    int npixels = pctx->npixels;
>> +    uint16_t* pdb0 = pctx->pdb0;
>> +    uint16_t* pcodebook = pctx->pcodebook;
>> +
>> +    int index;
>> +    while (npixels--)
>> +    {
>> +        index = *pinput++;
>> +        *pdb0++ = LE_16(&pcodebook[index]);
>> +    }
>> +}
>
> the 2 functions above address the array in fairly different ways why?

The array is addressed differently because the contents of the input  
data stream are different depending on the codec. codec0 basically  
says "This is a keyframe, just copy it", whereas codec6 says "Here is  
a bunch of 8-bit indices into a codebook, use them." codec0 has been  
replaced with a straight memcpy in accordance with your comment about  
endianness.

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

>> +
>> +static void fill_block(uint16_t* pdest, uint16_t color, int  
>> block_size, int pitch)
>> +{
>> +    int x, y;
>> +    pitch -= block_size;
>> +    for (y = 0; y != block_size; ++y, pdest += pitch)
>> +    {
>> +        for (x = 0; x != block_size; ++x)
>> +        {
>> +            *pdest++ = color;
>> +        }
>
> code duplication see msmpeg4_memsetw() / fill_db()

That function is static to msmpeg4.c, see comment above.

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

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

>> Property changes on: libavcodec/sanm.c
>> ___________________________________________________________________
>> Name: svn:keywords
>>    + Id URL
>> Name: svn:eol-style
>>    + native
>
> what is this?!

This crept in from my default svn client configuration. It makes it  
so the newline style for the file is adjusted according to the  
platform the developer is working on, to avoid messing up diffs  
because of newlines. The other property dynamically sets the URL of  
the file inside the file (if you insert a particular keyword like  
"$url$" or something, for convenience.) I'll remove it.

>> +typedef struct sanm_ainfo_t
>> +{
>> +    int freq, nchannels;
>
> why this duplication of AVCodecContext.sample_rate/channels?

There's no support for audio right now anyway. I'll do this later  
when audio can be integrated (right now can't do this because of  
audio API limitations.)

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

>> +            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 =)

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