[FFmpeg-devel] [PATCH] Add FITS Encoder

Reimar Döffinger Reimar.Doeffinger at gmx.de
Sat Jul 15 09:43:28 EEST 2017


On 14.07.2017, at 19:04, Paras Chadha <paraschadha18 at gmail.com> wrote:

> +static int write_keyword_value(char * header, const char * keyword, int value)
> +{
> +    int i, len, ret;
> +    len = strlen(keyword);
> +    for (i = 0; i < len; i++) {
> +        header[i] = keyword[i];
> +    }

would suggest memcpy (especially in combination with next suggestion).

> +    char header_line[80];

I would suggest memset to fill with spaces, to avoid most loops.

> +    if (fitsctx->first_image) {
> +        strncpy(header_line, "SIMPLE  = ", 10);
> +        for (i = 10; i < 80; i++) {
> +            header_line[i] = ' ';
> +        }
> +        header_line[29] = 'T';
> +    } else {
> +        strncpy(header_line, "XTENSION= 'IMAGE   '", 20);
> +        for (i = 20; i < 80; i++) {
> +            header_line[i] = ' ';
> +        }
> +    }

Never use strncpy, it has utterly broken behaviour that never makes sense.
av_strlcpy could be used, but just memcpy is probably best here.

> +    bytestream2_put_buffer(&pbc, header_line, 80);
> +    lines_written++;

Admittedly it provides some protection against buffer overflow
(but then, overflowing an on-stack buffer like header_line would be worse),
but it just feels wrong to construct the header in header_line just to then copy
it over into the packet.
It could just be written directly into the packet data, either manually or via bytestream2 functions (but using those memset with space probably can't be used to avoid a lot of code).

> +    write_keyword_value(header_line, "NAXIS", naxis);

Either here or maybe rather in the switch it might be good to explain a bit what the naxis stuff is used for.
For someone coming across the format for the first time, the number of components being called "axis" seems really weird.

> +    if (bitpix == 16) {
> +        write_keyword_value(header_line, "BZERO", bzero);
> +        bytestream2_put_buffer(&pbc, header_line, 80);
> +        lines_written++;
> +    }

This might also be worth a comment.

> +        switch (avctx->pix_fmt) {
> +        #define case_n(cas, dref) \

Macro definitions should start in column 0 (I think they actually have to?) and be in all-uppercase.

> +            case cas: \
> +                for (k = 0; k < naxis3; k++) { \
> +                    for (i = 0; i < avctx->height; i++) { \
> +                        ptr = p->data[0] + (avctx->height - i - 1) * p->linesize[0] + k; \
> +                        for (j = 0; j < avctx->width; j++) { \
> +                            bytestream2_put_byte(&pbc, dref(ptr) - bzero); \
> +                            ptr += naxis3; \
> +                        } \
> +                    } \
> +                } \
> +                break
> +            case_n(AV_PIX_FMT_RGB24, *);
> +            case_n(AV_PIX_FMT_RGBA, *);
> +            case_n(AV_PIX_FMT_RGB48BE, AV_RB16);
> +            case_n(AV_PIX_FMT_RGBA64BE, AV_RB16);

The macro will generate the same code for RGB24 and RGBA, and respectively for the other 2 cases.
Doing this without a macro and merging the cases with identical code seems much better.

> +            if (bitpix == 16) {
> +                for (j = 0; j < avctx->width; j++) {
> +                    bytestream2_put_be16(&pbc, AV_RB16(ptr) - bzero);
> +                    ptr += 2;
> +                }

Can't bzero be chosen as 0? Because then this is just a normal memcpy...

> +    t = padded_data_size - data_size;
> +    while (t--) {
> +        bytestream2_put_byte(&pbc, 0);
> +    }

Might be worth getting the bytestream pointer and memset (or adding memset functionality to bytestream2).


More information about the ffmpeg-devel mailing list