[FFmpeg-devel] [PATCH] Add DPX decoder rev-11

[Michael Niedermayer]

On Thu, May 28, 2009 at 07:19:48AM +0200, Jimmy Christensen wrote:
> On 2009-05-27 21:18, Vitor Sessak wrote:
>> Jimmy Christensen wrote:
>>> On 2009-05-27 14:30, Michael Niedermayer wrote:
>>>> On Tue, May 26, 2009 at 05:51:25AM +0200, Jimmy Christensen wrote:
>>>>> On 2009-05-25 23:58, Michael Niedermayer wrote:
>>>>>> On Mon, May 25, 2009 at 08:54:12AM +0200, Jimmy Christensen wrote:
>>>>>>> On 2009-05-15 03:51, Michael Niedermayer wrote:
>>>>>>>> On Mon, May 11, 2009 at 11:31:25AM +0200, Jimmy Christensen wrote:
>>>>>> [...]
>>>>>>>>
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>> + for (x = 0; x< s->height; x++) {
>>>>>>>>> + uint8_t *dst = ptr;
>>>>>>>>> + for (y = 0; y< s->width; y++) {
>>>>>>>>
>>>>>>>>> + rgbBuffer = AV_RB32(buf);
>>>>>>>>> + memcpy(&rgb10Field,&rgbBuffer, 4);
>>>>>>>>> + rgb16Field.R = rgb10Field.R * 64; // 10-bit> 16-bit
>>>>>>>>> + rgb16Field.G = rgb10Field.G * 64; // 10-bit> 16-bit
>>>>>>>>> + rgb16Field.B = rgb10Field.B * 64; // 10-bit> 16-bit
>>>>>>>>> + memcpy(dst,&rgb16Field, dstBpp);
>>>>>>>>
>>>>>>>> not portable
>>>>>>>>
>>>>>>>
>>>>>>> I rewrote a little, but still uses memcpy from rgb16Field to dst,
>>>>>>> since
>>>>>>> that should actually be portable.
>>>>>>
>>>>>> you will have to quote page and pararaph of the C standard that
>>>>>> gurantees
>>>>>> bitfields to work the way you seem to belive they do
>>>>>>
>>>>>
>>>>> So the bit masking is fine (RED10, GREEN10 and BLUE10), but I need to
>>>>> change the memcpy?
>>>>
>>>> we need C code that works, any construct that has undefined or
>>>> implementation
>>>> defined behavior is unacceptable. That is with a few exceptions that
>>>> happen
>>>> to be exceedingly inconvenient to avoid or would cause speedloss and
>>>> happen
>>>> to work on all for us relevant implementations, like
>>>> signed>> or CHAR_BIT == 8
>>>>
>>>
>>> I'm not familiar with coding on other platforms and what code is
>>> portable or not. I'm just seeing that it works here and try to get
>>> some input on how to fix the code to make it portable. So forgive my
>>> lack of knowledge. I was merely asking for a yes or no.
>>
>> I suggest you remove completely the RGB16Field struct and just use
>>
>>> + for (x = 0; x < s->height; x++) {
>>> + uint16_t *dst = ptr;
>>
>> instead of
>>
>>> + for (x = 0; x < s->height; x++) {
>>> + uint8_t *dst = ptr;
>>
>> as a secondary benefit, the code should be simpler (no memcpy()).
>>
>> -Vitor
>
> Thank you. Didn't know about this.
>
> Solved my problem and the code should be alot better and cleaner. Although 
> there is 1 warning about changing format from uint8_t to uint16_t. It seems 
> that avctx->priv_data->picture->data is always uint8_t so either way it 
> seems I'll get a warning no matter what. Also since 
> avctx->priv_data->picture->linesize is based on a uint8_t I'll need it as 
> uint8_t anyway.
>

> I also forced the avctx->pix_fmt to PIX_FMT_RGB48BE to avoid any portable 
> issues and also since this is the only format which is supported by any 
> encoders(pnm).

the output should be PIX_FMT_RGB48

[...]
> +struct RGB16Field {
> +    unsigned R :16;
> +    unsigned G :16;
> +    unsigned B :16;
> +};

unused


> +
> +typedef struct DPXContext {
> +    AVFrame picture;
> +    int width;
> +    int height;

> +    int color_type;
> +    int compression_type

unused


> +} DPXContext;
> +
> +
> +static unsigned int read32(const uint8_t **ptr, int is_big)
> +{
> +    unsigned int temp;
> +    if(is_big)
> +        temp = AV_RB32(*ptr);
> +    else
> +        temp = AV_RL32(*ptr);
> +    *ptr += 4;
> +    return temp;
> +}
> +

> +static void write16(uint16_t **ptr, unsigned int color)
> +{
> +    AV_WB16(*ptr, color);
> +    *ptr += 1;
> +}

duplicate of bytestream_put_be16()


[...]
> +    offset = read32(&headerBuffer, endian);
> +    bytestream_get_buffer(&headerBuffer, version, 8);
> +    // Need to end in 0x300, so jump 768 - 8 - 4 - 4 = 752
> +    headerBuffer += 752;

> +    orientation = read32(&headerBuffer, endian);

unused

[...]
> +    for (x = 0; x < s->height; x++) {
> +        uint16_t *dst = ptr;
> +        for (y = 0; y < s->width; y++) {
> +            rgbBuffer = AV_RB32(buf);

> +            red16   = RED10(rgbBuffer) * 64; // 10-bit > 16-bit
> +            green16 = GREEN10(rgbBuffer) * 64; // 10-bit > 16-bit
> +            blue16  = BLUE10(rgbBuffer) * 64; // 10-bit > 16-bit

there are 3 avoidable operations in there at least


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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090528/75dcc453/attachment.pgp>