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

Cyril Zorin cyril.zorin
Fri Mar 2 20:19:42 CET 2007


Is there a helper function that runs through a buffer and flips every  
16-bit value in it? I need this for converting the final output image  
to native endian. I could write the function myself, but I'm  
wondering if there's something like this already.

Thanks =)

On 27-Feb-07, at 6:47 PM, Michael Niedermayer wrote:

> 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
> _______________________________________________
> 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