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

Michael Niedermayer michaelni
Mon Jun 8 16:12:52 CEST 2009


On Mon, Jun 08, 2009 at 12:17:32AM +0200, Jimmy Christensen wrote:
> On 2009-06-05 00:00, Jimmy Christensen wrote:
>> On 2009-06-04 23:28, Vitor Sessak wrote:
>>> Jimmy Christensen wrote:
>>>> On 2009-06-04 18:33, Vitor Sessak wrote:
>>>>> Jimmy Christensen wrote:
>>>>> [....]
>>>>> 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;
>>>>>
>>>>
>>>> Hmm.. I'm not so sure it's a good idea. First of all I think the idea
>>>> of the pix_fmt is to have it as the systems native endian.
>>>
>>> Not really. The general idea is that the decoder should not do pix fmt
>>> conversion (this is swscaler task). Of course, there are exceptions to
>>> avoid polluting the API with too many sorts of weird formats (like 10
>>> bit RGB, for example). If the decoder can output to two different
>>> formats (as in the 10-bit case), it should use the one that it decodes
>>> to faster (what in general favors native endianness).
>>>
>>>> Also in my test case it didn't make any difference performance wise,
>>>> probably bacause the rgb48 > yuv has to do endian conversion anyway.
>>>
>>> Imagine in a LE box do a conversion where the input is DPX 16BE and the
>>> encoder wants RGB16BE (PCX?, TIFF?, some not yet existing stuff?). Your
>>> code will convert 16BE to 16LE and the scaler back to 16BE.
>>
>> Very true, but I imagine that the most used cases would be to another
>> format completely. Personally if I were to convert/resize from one image
>> format to another eg. TIFF I would probably use imagemagick instead of
>> ffmpeg (no offense there :) ). Also there aren't many formats out there
>> which support 16-bit and all of those I have seen so far supports both
>> little and big endian. The output would then naturally be the systems
>> native endian.
>>
>>>
>>>> This will also be the case when/if I have to do log/lin conversion.
>>>
>>> For log/lin conversion, I agree that native endianness is better
>>> (faster).
>>>
>>
>> I plan on making the log/lin conversion through a videofilter, so it's
>> also possible to support LUTs. I think this will probably be more used
>> compared to image > image.
>>
>> The decision is up to you guys. I can easily make the change.
>
> I've now changed it to match the source image endian type, which does in 
> fact make some sense. One can argue that due to this, having the files 
> endian type match the systems will always be the fastest.
>
> Also added support for RGBA although discards the alpha channel for all bit 
> depths except 8-bit.
[...]
> +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, descriptor, elements, target_packet_size, source_packet_size;
> +
> +    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\n");
> +        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 + 0x320;

buf+= 8 is a lot clearer here IMHO

[...]
> +    switch (bits_per_color) {
> +        case 8:
> +            if (elements == 4)
> +                avctx->pix_fmt = PIX_FMT_RGBA;
> +            else
> +                avctx->pix_fmt = PIX_FMT_RGB24;
> +            source_packet_size = elements;
> +            target_packet_size = elements;
> +            break;
> +        case 10:
> +        case 12:
> +        case 16:
> +            if (endian)
> +                avctx->pix_fmt = PIX_FMT_RGB48LE;
> +            else
> +                avctx->pix_fmt = PIX_FMT_RGB48BE;
> +            target_packet_size = 6;
> +            source_packet_size = elements * 2;
> +            break;
> +        default:
> +            av_log(avctx, AV_LOG_ERROR, "Unsupported color depth\n");
> +            return -1;
> +            break;
> +    }
> +
> +    if (s->picture.data[0])
> +        avctx->release_buffer(avctx, &s->picture);
> +    if (avcodec_check_dimensions(avctx, w, h))
> +        return -1;
> +    if (w != avctx->width || h != avctx->height)
> +        avcodec_set_dimensions(avctx, w, h);
> +    if (avctx->get_buffer(avctx, p) < 0) {
> +        av_log(avctx, AV_LOG_ERROR, "get_buffer() failed\n");
> +        return -1;
> +    }
> +
> +    // Move pointer to offset from start of file
> +    buf =  avpkt->data + offset;
> +
> +    ptr    = p->data[0];
> +    stride = p->linesize[0];
> +
> +    switch (bits_per_color) {
> +        case 10:
> +            for (x = 0; x < avctx->height; x++) {
> +               uint16_t *dst = ptr;
> +               for (y = 0; y < avctx->width; y++) {
> +                   rgbBuffer = read32(&buf, endian);
> +                   // Read out the 10-bit colors and convert to 16-bit
> +                   *dst++ = make_16bit(rgbBuffer >> 16);
> +                   *dst++ = make_16bit(rgbBuffer >>  6);
> +                   *dst++ = make_16bit(rgbBuffer <<  4);
> +               }
> +               ptr += stride;
> +            }
> +            break;
> +        case 8:
> +        case 12: // Treat 12-bit as 16-bit
> +        case 16:
> +            if (source_packet_size == target_packet_size) {
> +                for (x = 0; x < avctx->height; x++) {
> +                    memcpy(ptr, buf, target_packet_size*avctx->width);
> +                    ptr += stride;
> +                    buf += source_packet_size*avctx->width;
> +                }
> +            } else {
> +                for (x = 0; x < avctx->height; x++) {
> +                    uint8_t *dst = ptr;
> +                    for (y = 0; y < avctx->width; y++) {
> +                        memcpy(dst, buf, target_packet_size);
> +                        dst += target_packet_size;
> +                        buf += source_packet_size;
> +                    }
> +                    ptr += stride;
> +                }
> +            }
> +            break;
> +        default:
> +            return -1;
> +            break;
> +    }

this code looks buggy, mixing LE & BE up

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

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20090608/7ccb0d9a/attachment.pgp>



More information about the ffmpeg-devel mailing list