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

[Vitor Sessak]

Jimmy Christensen wrote:
> On 2009-06-04 15:00, Diego Biurrun wrote:
>> On Thu, Jun 04, 2009 at 02:55:08PM +0200, Jimmy Christensen wrote:
>>> On 2009-06-04 13:31, Michael Niedermayer wrote:
>>>>
>>>>> +    bpc = AV_RB16(buf);
>>>>
>>>> what is bpc? i think this is not a good name for a variable
>>>
>>> Bits per color. Renamed it to bitsPerColor.
>>
>> Could we please have this without the ugly camelcasing, e.g.
>> bits_per_color?
>>
> 
> Renamed. Guess I get that habit from my main line of work :)

Just an idea:

>>> --- libavcodec/dpx.c    (revision 0)
>>> +++ libavcodec/dpx.c    (revision 0)
>>> @@ -0,0 +1,187 @@
>>> +    const uint8_t *buf          = avpkt->data;
>>> +    int buf_size                = avpkt->size;
>>
>> nit: This looks slightly odd moved so much to the right.
>>
> 
> Think it was due to some names I had earlier which were removed.

Hmm, in the following:

> +
> +static int decode_frame(AVCodecContext *avctx,
> +                        void *data,
> +                        int *data_size,
> +                        AVPacket *avpkt)
> +{
> +    const uint8_t *buf = avpkt->data;
> +    int buf_size       = avpkt->size;
> +    DPXContext *const s = avctx->priv_data;
> +    AVFrame *picture  = data;
> +    AVFrame *const p = &s->picture;
> +    uint8_t *ptr;
> +
> +    int magic_num, offset, endian;
> +    int x, y;
> +    int w, h, stride, bits_per_color;
> +
> +    unsigned int rgbBuffer;
> +
> +    magic_num = AV_RB32(buf);
> +    buf += 4;
> +
> +    /* Check if the files "magic number" is "SDPX" which means it uses
> +     * big-endian or XPDS which is for little-endian files */
> +    if (magic_num == AV_RL32("SDPX"))
> +        endian = 0;
> +    else if (magic_num == AV_RB32("SDPX"))
> +        endian = 1;
> +    else {
> +        av_log(avctx, AV_LOG_ERROR, "DPX marker not found");
> +        return -1;
> +    }
> +
> +    offset = read32(&buf, endian);
> +    // Need to end in 0x304 offset from start of file
> +    buf = avpkt->data + 0x304;
> +    w = read32(&buf, endian);
> +    h = read32(&buf, endian);
> +
> +    buf = avpkt->data + 0x322;
> +    bits_per_color = AV_RB16(buf);
> +
> +    avctx->pix_fmt = PIX_FMT_RGB48;

If you make something like

if (bcc == 16)
     avctx->pix_fmt = endian ? PIX_FMT_RGB48BE : PIX_FMT_RGB48LE;
else
     avctx->pix_fmt = PIX_FMT_RGB48;

you can replace

> +        case 16:
> +            for (x = 0; x < avctx->height; x++) {
> +               uint16_t *dst = ptr;
> +               for (y = 0; y < avctx->width; y++) {
> +                   *dst++ = read16(&buf, endian);
> +                   *dst++ = read16(&buf, endian);
> +                   *dst++ = read16(&buf, endian);
> +               }
> +               ptr += stride;
> +            }
> +            break;

by just

         case 16:
             for (x = 0; x < avctx->height; x++) {
                memcpy(ptr, buf, 6*avctx->width);
                ptr += stride;
             }
             break;

-Vitor