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

[Vitor Sessak]

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