[FFmpeg-devel] [PATCH v7 1/2] libavcodec/pgxdec: Add PGX decoder

[Andreas Rheinhardt]

Nicolas George:
> gautamramk at gmail.com (12020-07-03):
>> From: Gautam Ramakrishnan <gautamramk at gmail.com>
>>
>> This patch adds a pgx decoder.
> 
> Reviewing even after push, because there are problems to fix.
> 
>> ---
>>  Changelog               |   1 +
>>  doc/general.texi        |   2 +
>>  libavcodec/Makefile     |   1 +
>>  libavcodec/allcodecs.c  |   1 +
>>  libavcodec/codec_desc.c |   7 ++
>>  libavcodec/codec_id.h   |   1 +
>>  libavcodec/pgxdec.c     | 169 ++++++++++++++++++++++++++++++++++++++++
>>  libavcodec/version.h    |   4 +-
>>  8 files changed, 184 insertions(+), 2 deletions(-)
>>  create mode 100644 libavcodec/pgxdec.c
>>
[...]
>> +#define WRITE_FRAME(D, PIXEL, suffix)                                                       \
>> +    static inline void write_frame_ ##D(AVPacket *avpkt, AVFrame *frame, GetByteContext *g, \
>> +                                        int width, int height, int sign, int depth)         \
>> +    {                                                                                       \
>> +        int i, j;                                                                           \
>> +        for (i = 0; i < height; i++) {                                                      \
> 
>> +            PIXEL *line = (PIXEL*)frame->data[0] + i*frame->linesize[0]/sizeof(PIXEL);      \
> 
> It would have been more elegant and more efficient to keep the
> incrementation of line at the end of the loop than to keep its
> initialization here.
> 
>> +            for (j = 0; j < width; j++) {                                                   \
>> +                int val;                                                                    \
>> +                if (sign)                                                                   \
> 
>> +                    val = (PIXEL)bytestream2_get_ ##suffix(g) + (1 << (depth - 1));         \
> 
> Casting unsigned to negative signed is not portable.
> 

It's not completely portable, but FFmpeg does not aim for complete
portability: "Implementation defined behavior for signed integers is
assumed to match the expected behavior for two’s complement". (see
https://ffmpeg.org/developer.html#C-language-features)

>> +                else                                                                        \
>> +                    val = bytestream2_get_ ##suffix(g);                                     \
>> +                val <<= (D - depth);                                                        \
> 
>> +                *(line + j) = val;                                                          \
> 
> *(line + j) is almost always better written line[j]
> 
>> +            }                                                                               \
>> +        }                                                                                   \
>> +    }                                                                                       \
>> +
>> +WRITE_FRAME(8, int8_t, byte)
>> +WRITE_FRAME(16, int16_t, be16)
>> +

- Andreas