[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