[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