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

[Jimmy Christensen]

On 2009-06-02 22:21, Michael Niedermayer wrote:
> On Tue, Jun 02, 2009 at 10:18:02AM +0200, Jimmy Christensen wrote:
>> On 2009-06-02 10:09, Jimmy Christensen wrote:
>>> On 2009-06-02 10:06, Diego Biurrun wrote:
>>>> On Tue, Jun 02, 2009 at 09:16:06AM +0200, Jimmy Christensen wrote:
>>>>> --- Changelog (revision 18866)
>>>>> +++ Changelog (working copy)
>>>>> @@ -18,6 +18,7 @@
>>>>> - 8088flex TMV demuxer and decoder
>>>>> - per-stream language-tags extraction in asfdec
>>>>> - V210 decoder and encoder
>>>>> +- DPX image decoder
>>>>
>>>> This is not HEAD.
>>>>
>>>> Diego
>>>
>>> I know. The problem is somewhat that I need to use a RGB48 patch which
>>> doesn't work with HEAD to test the decoder. If the patch is OK'ed I will
>>> gladly make a patch which is compatible with HEAD.
>>
>> Made a new patch which works with HEAD.
>
> [...]
>> +typedef struct DPXContext {
>> +    AVFrame picture;
>
>> +    int width;
>> +    int height;
>

Thanks. Will remove those and change the code accordingly.

> redundant with AVCodecContext.width/...
>
> [...]
>> +            if(avctx->pix_fmt == PIX_FMT_RGB48BE) {
>> +                bytestream_put_be16(&dst, MAKE10to16(RED10(rgbBuffer)));
>> +                bytestream_put_be16(&dst, MAKE10to16(GREEN10(rgbBuffer)));
>> +                bytestream_put_be16(&dst, MAKE10to16(BLUE10(rgbBuffer)));
>> +            } else {
>> +                bytestream_put_le16(&dst, MAKE10to16(RED10(rgbBuffer)));
>> +                bytestream_put_le16(&dst, MAKE10to16(GREEN10(rgbBuffer)));
>> +                bytestream_put_le16(&dst, MAKE10to16(BLUE10(rgbBuffer)));
>
> this contains at least 6 unneeded shifts
>
Like this instead ?

*dst++ = MAKE10to16(RED10(rgbBuffer));
*dst++ = MAKE10to16(GREEN10(rgbBuffer));
*dst++ = MAKE10to16(BLUE10(rgbBuffer));

or do you mean combining the MAKE10to16 with RED10? Personally I think 
that would make the code less readable.