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

[Jimmy Christensen]

On 2009-05-28 12:52, Michael Niedermayer wrote:
> On Thu, May 28, 2009 at 07:19:48AM +0200, Jimmy Christensen wrote:
>> On 2009-05-27 21:18, Vitor Sessak wrote:
>>> [...]
>>>
>>> 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

The thing is that PIX_FMT_RGB48 becomes PIX_FMT_RGB48BE on big endian 
systems and PIX_FMT_RGB48LE on little endian systems. Tried to avoid 
having a if else thing, but probably makes more sense to have the 
picture frame in the systems native format. Changed it in this patch. 
The problem now is that without the RGB48 support patch for swscale. The 
dpx decoder can only be used on big endian systems, since there are no 
PIX_FMT_RGB48LE supporting encoders. The pnm encoder specifically 
chooses PIX_FMT_RGB48BE.

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

Forgot to remove that one...

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

Removed.

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

Didn't know about that one. Thanks. Used that instead.

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

Removed although I would still have liked to have it there for future 
implementation of the orientation field.

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

Changed
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ffmpegDPX-rev12.diff
Type: text/x-patch
Size: 8578 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090529/1b257472/attachment.bin>