diff mbox

[FFmpeg-devel] Add FITS Encoder

Message ID 1500208280-8475-1-git-send-email-paraschadha18@gmail.com
State Superseded
Headers show

Commit Message

Paras July 16, 2017, 12:31 p.m. UTC
Signed-off-by: Paras Chadha <paraschadha18@gmail.com>
---
 doc/general.texi       |   2 +
 libavcodec/Makefile    |   1 +
 libavcodec/allcodecs.c |   1 +
 libavcodec/fitsenc.c   | 238 +++++++++++++++++++++++++++++++++++++++++++++++++
 libavformat/img2enc.c  |   1 +
 5 files changed, 243 insertions(+)
 create mode 100644 libavcodec/fitsenc.c

Comments

Derek Buitenhuis July 16, 2017, 12:59 p.m. UTC | #1
On 7/16/2017 1:31 PM, Paras Chadha wrote:
> Signed-off-by: Paras Chadha <paraschadha18@gmail.com>
> ---
>  doc/general.texi       |   2 +
>  libavcodec/Makefile    |   1 +
>  libavcodec/allcodecs.c |   1 +
>  libavcodec/fitsenc.c   | 238 +++++++++++++++++++++++++++++++++++++++++++++++++
>  libavformat/img2enc.c  |   1 +
>  5 files changed, 243 insertions(+)
>  create mode 100644 libavcodec/fitsenc.c

Why is there no decoder?

Also, missing configure change (?) and version bump.

> +
> +static int write_keyword_value(uint8_t **bytestream, const char * keyword, int value)
> +{
> +    int len, ret;
> +    uint8_t * header = * bytestream;

Please don't put spaces between '*' for pointers, it's pretty ambiguous to readers.

> +    len = strlen(keyword);
> +
> +    memcpy(header, keyword, len);
> +    memset(header + len, ' ', 8 - len);

This sort of stuff makes me unfortable, even if the input is all literals. At
the very least, it should be documented, or checked, or a constant used instead
of a magic 8. Maybe I'm just paranoid.

> +    data_size = (bitpix >> 3) * avctx->height * avctx->width * naxis3;

That's this based off of?

- Derek
Nicolas George July 16, 2017, 1:08 p.m. UTC | #2
L'octidi 28 messidor, an CCXXV, Derek Buitenhuis a écrit :
> Why is there no decoder?

Because the decoder is in a separate patch, and the demuxer in yet
another. And none of them can proceed until Paras Chadha starts deciding
what is considered part of the codec and what is considered part of the
format.

This looks like a common mistake by students: code, code, code, when
they should be stopping and designing. I do not know who Paras Chadha's
mentor is.

Regards,
Paras July 16, 2017, 2:34 p.m. UTC | #3
On Sun, Jul 16, 2017 at 6:29 PM, Derek Buitenhuis <
derek.buitenhuis@gmail.com> wrote:

> On 7/16/2017 1:31 PM, Paras Chadha wrote:
> > Signed-off-by: Paras Chadha <paraschadha18@gmail.com>
> > ---
> >  doc/general.texi       |   2 +
> >  libavcodec/Makefile    |   1 +
> >  libavcodec/allcodecs.c |   1 +
> >  libavcodec/fitsenc.c   | 238 ++++++++++++++++++++++++++++++
> +++++++++++++++++++
> >  libavformat/img2enc.c  |   1 +
> >  5 files changed, 243 insertions(+)
> >  create mode 100644 libavcodec/fitsenc.c
>
> Why is there no decoder?
>
> Also, missing configure change (?) and version bump.
>

Decoder is in separate patch


>
> > +
> > +static int write_keyword_value(uint8_t **bytestream, const char *
> keyword, int value)
> > +{
> > +    int len, ret;
> > +    uint8_t * header = * bytestream;
>
> Please don't put spaces between '*' for pointers, it's pretty ambiguous to
> readers.
>

ok


>
> > +    len = strlen(keyword);
> > +
> > +    memcpy(header, keyword, len);
> > +    memset(header + len, ' ', 8 - len);
>
> This sort of stuff makes me unfortable, even if the input is all literals.
> At
> the very least, it should be documented, or checked, or a constant used
> instead
> of a magic 8. Maybe I'm just paranoid.
>

8 is used because it is the maximum size of a header keyword. All the
string literals i am passing in keyword have length less than or equal to 8
and greater than 0. Still, i will add a check that it is executed only when
len > 0 and len <= 8 and use constant instead of 8.


>
> > +    data_size = (bitpix >> 3) * avctx->height * avctx->width * naxis3;
>
> That's this based off of?
>

I am sorry, i didn't get the question.


>
> - Derek
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Reimar Döffinger July 16, 2017, 5 p.m. UTC | #4
On 16.07.2017, at 14:31, Paras Chadha <paraschadha18@gmail.com> wrote:
> +static int write_keyword_value(uint8_t **bytestream, const char * keyword, int value)
> +{
> +    int len, ret;
> +    uint8_t * header = * bytestream;
> +    len = strlen(keyword);
> +
> +    memcpy(header, keyword, len);
> +    memset(header + len, ' ', 8 - len);
> +    header[8] = '=';
> +    header[9] = ' ';
> +    header += 10;
> +
> +    ret = snprintf(header, 70, "%d", value);
> +    memset(header + ret, ' ', 70 - ret);

You are unnecessarily complicating this.
Start with memset(header, ' ', 80); and remove all the other memsets (also applies to code further down).
Then only fix up the snprintf 0-termination character.

> +        memcpy(bytestream, "XTENSION= 'IMAGE   '", 20);

Not really sure it is better, so at your discretion, but there are 2 alternatives to doing this (the variable names are probably bad and just for demonstration):
// explicit size dropping 0-termination
static const image_ext_str[20] = "XTENSION= 'IMAGE   '";
memcpy(bytestream, image_ext_str, sizeof(image_ext_str));

or:

static const image_ext_str[] = "XTENSION= 'IMAGE   '";
// -1 to drop 0-termination
memcpy(bytestream, image_ext_str, sizeof(image_ext_str) - 1);

The advantage is that it avoids the "magic" numbers which are easy to get wrong.
Reimar Döffinger July 16, 2017, 5:04 p.m. UTC | #5
On 16.07.2017, at 15:08, Nicolas George <george@nsup.org> wrote:

> L'octidi 28 messidor, an CCXXV, Derek Buitenhuis a écrit :
>> Why is there no decoder?
> 
> Because the decoder is in a separate patch, and the demuxer in yet
> another. And none of them can proceed until Paras Chadha starts deciding
> what is considered part of the codec and what is considered part of the
> format.
> 
> This looks like a common mistake by students: code, code, code, when
> they should be stopping and designing. I do not know who Paras Chadha's
> mentor is.

Well, I wouldn't take it so seriously.
At least the things I've reviewed it doesn't really matter much.
Also I've seen students about as much sit and design, discuss, etc when they should be coding, because they lacked the experience and understanding of the problem for all their discussion and  designing to really give useful results.
Maybe you know different students or you just don't see that side. But either way, students are studying for a reason (hopefully) ;-).
diff mbox

Patch

diff --git a/doc/general.texi b/doc/general.texi
index 8f582d5..7f389ce 100644
--- a/doc/general.texi
+++ b/doc/general.texi
@@ -591,6 +591,8 @@  following image formats are supported:
     @tab Digital Picture Exchange
 @item EXR          @tab   @tab X
     @tab OpenEXR
+@item FITS         @tab X @tab X
+    @tab Flexible Image Transport System
 @item JPEG         @tab X @tab X
     @tab Progressive JPEG is not supported.
 @item JPEG 2000    @tab X @tab X
diff --git a/libavcodec/Makefile b/libavcodec/Makefile
index de1187f..9ad44e0 100644
--- a/libavcodec/Makefile
+++ b/libavcodec/Makefile
@@ -291,6 +291,7 @@  OBJS-$(CONFIG_FFV1_ENCODER)            += ffv1enc.o ffv1.o
 OBJS-$(CONFIG_FFWAVESYNTH_DECODER)     += ffwavesynth.o
 OBJS-$(CONFIG_FIC_DECODER)             += fic.o
 OBJS-$(CONFIG_FITS_DECODER)            += fitsdec.o
+OBJS-$(CONFIG_FITS_ENCODER)            += fitsenc.o
 OBJS-$(CONFIG_FLAC_DECODER)            += flacdec.o flacdata.o flac.o
 OBJS-$(CONFIG_FLAC_ENCODER)            += flacenc.o flacdata.o flac.o vorbis_data.o
 OBJS-$(CONFIG_FLASHSV_DECODER)         += flashsv.o
diff --git a/libavcodec/allcodecs.c b/libavcodec/allcodecs.c
index 0243f47..d180dde 100644
--- a/libavcodec/allcodecs.c
+++ b/libavcodec/allcodecs.c
@@ -192,6 +192,7 @@  static void register_all(void)
     REGISTER_ENCDEC (FFV1,              ffv1);
     REGISTER_ENCDEC (FFVHUFF,           ffvhuff);
     REGISTER_DECODER(FIC,               fic);
+    REGISTER_ENCDEC (FITS,              fits);
     REGISTER_ENCDEC (FLASHSV,           flashsv);
     REGISTER_ENCDEC (FLASHSV2,          flashsv2);
     REGISTER_DECODER(FLIC,              flic);
diff --git a/libavcodec/fitsenc.c b/libavcodec/fitsenc.c
new file mode 100644
index 0000000..cc3fe77
--- /dev/null
+++ b/libavcodec/fitsenc.c
@@ -0,0 +1,238 @@ 
+/*
+ * FITS image encoder
+ * Copyright (c) 2017 Paras Chadha
+ *
+ * This file is part of FFmpeg.
+ *
+ * FFmpeg is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * FFmpeg is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with FFmpeg; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+ */
+
+/**
+ * @file
+ * FITS image encoder
+ *
+ * Specification: https://fits.gsfc.nasa.gov/fits_standard.html Version 3.0
+ *
+ * RGBA images are encoded as planes in RGBA order. So, NAXIS3 is 3 or 4 for them.
+ * Also CTYPE = 'RGB ' is added to the header to distinguish them from 3d images.
+ */
+
+#include "libavutil/intreadwrite.h"
+#include "avcodec.h"
+#include "bytestream.h"
+#include "internal.h"
+
+typedef struct FITSContext {
+    int first_image;
+} FITSContext;
+
+static av_cold int fits_encode_init(AVCodecContext *avctx)
+{
+    FITSContext * fitsctx = avctx->priv_data;
+    fitsctx->first_image = 1;
+    return 0;
+}
+
+static int write_keyword_value(uint8_t **bytestream, const char * keyword, int value)
+{
+    int len, ret;
+    uint8_t * header = * bytestream;
+    len = strlen(keyword);
+
+    memcpy(header, keyword, len);
+    memset(header + len, ' ', 8 - len);
+    header[8] = '=';
+    header[9] = ' ';
+    header += 10;
+
+    ret = snprintf(header, 70, "%d", value);
+    memset(header + ret, ' ', 70 - ret);
+
+    *bytestream += 80;
+    return 0;
+}
+
+static int fits_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
+                            const AVFrame *pict, int *got_packet)
+{
+    AVFrame * const p = (AVFrame *)pict;
+    FITSContext * fitsctx = avctx->priv_data;
+    uint8_t *bytestream, *bytestream_start, *ptr;
+    uint64_t header_size = 2880, data_size = 0, padded_data_size = 0;
+    int ret, bitpix, naxis, naxis3 = 1, bzero = 0, i, j, k, t, rgb = 0;
+
+    switch (avctx->pix_fmt) {
+        case AV_PIX_FMT_GRAY8:
+            bitpix = 8;
+            naxis = 2;
+            break;
+        case AV_PIX_FMT_GRAY16BE:
+            bitpix = 16;
+            naxis = 2;
+            bzero = 32768;
+            break;
+        case AV_PIX_FMT_RGB24:
+        case AV_PIX_FMT_RGBA:
+            bitpix = 8;
+            naxis = 3;
+            rgb = 1;
+            if (avctx->pix_fmt == AV_PIX_FMT_RGB24) {
+                naxis3 = 3;
+            } else {
+                naxis3 = 4;
+            }
+            break;
+        case AV_PIX_FMT_RGB48BE:
+        case AV_PIX_FMT_RGBA64BE:
+            bitpix = 16;
+            naxis = 3;
+            bzero = 32768;
+            rgb = 1;
+            if (avctx->pix_fmt == AV_PIX_FMT_RGB48BE) {
+                naxis3 = 3;
+            } else {
+                naxis3 = 4;
+            }
+            break;
+        default:
+            av_log(avctx, AV_LOG_ERROR, "unsupported pixel format\n");
+            return AVERROR(EINVAL);
+    }
+
+    data_size = (bitpix >> 3) * avctx->height * avctx->width * naxis3;
+    padded_data_size = ((data_size + 2879) / 2880 ) * 2880;
+
+    if ((ret = ff_alloc_packet2(avctx, pkt, header_size + padded_data_size, 0)) < 0)
+        return ret;
+
+    bytestream_start =
+    bytestream       = pkt->data;
+
+    if (fitsctx->first_image) {
+        memcpy(bytestream, "SIMPLE  = ", 10);
+        memset(bytestream + 10, ' ', 70);
+        bytestream[29] = 'T';
+    } else {
+        memcpy(bytestream, "XTENSION= 'IMAGE   '", 20);
+        memset(bytestream + 20, ' ', 60);
+    }
+    bytestream += 80;
+
+    write_keyword_value(&bytestream, "BITPIX", bitpix);         // no of bits per pixel
+    write_keyword_value(&bytestream, "NAXIS", naxis);           // no of dimensions of image
+    write_keyword_value(&bytestream, "NAXIS1", avctx->width);   // first dimension i.e. width
+    write_keyword_value(&bytestream, "NAXIS2", avctx->height);  // second dimension i.e. height
+
+    if (rgb)
+        write_keyword_value(&bytestream, "NAXIS3", naxis3);     // third dimension to store RGBA planes
+
+    if (!fitsctx->first_image) {
+        write_keyword_value(&bytestream, "PCOUNT", 0);
+        write_keyword_value(&bytestream, "GCOUNT", 1);
+    } else {
+        fitsctx->first_image = 0;
+    }
+
+    /*
+     * Since FITS does not support unsigned 16 bit integers,
+     * BZERO = 32768 is used to store unsigned 16 bit integers as
+     * signed integers so that it can be read properly.
+     */
+    if (bitpix == 16)
+        write_keyword_value(&bytestream, "BZERO", bzero);
+
+    if (rgb) {
+        memcpy(bytestream, "CTYPE3  = 'RGB     '", 20);
+        memset(bytestream + 20, ' ', 60);
+        bytestream += 80;
+    }
+
+    memcpy(bytestream, "END", 3);
+    memset(bytestream + 3, ' ', 77);
+    bytestream += 80;
+
+    t = header_size - (bytestream - bytestream_start);
+    memset(bytestream, ' ', t);
+    bytestream += t;
+
+    if (rgb) {
+        switch (avctx->pix_fmt) {
+            case AV_PIX_FMT_RGB24:
+            case AV_PIX_FMT_RGBA:
+                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++) {
+                            bytestream_put_byte(&bytestream, ptr[0]);
+                            ptr += naxis3;
+                        }
+                    }
+                }
+                break;
+            case AV_PIX_FMT_RGB48BE:
+            case AV_PIX_FMT_RGBA64BE:
+                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 * 2;
+                        for (j = 0; j < avctx->width; j++) {
+                            bytestream_put_be16(&bytestream, AV_RB16(ptr) - bzero);
+                            ptr += naxis3 * 2;
+                        }
+                    }
+                }
+                break;
+        }
+    } else {
+        for (i = 0; i < avctx->height; i++) {
+            ptr = p->data[0] + (avctx->height - i - 1) * p->linesize[0];
+            if (bitpix == 16) {
+                for (j = 0; j < avctx->width; j++) {
+                    bytestream_put_be16(&bytestream, AV_RB16(ptr) - bzero);
+                    ptr += 2;
+                }
+            } else {
+                memcpy(bytestream, ptr, avctx->width);
+                bytestream += avctx->width;
+            }
+        }
+    }
+
+    t = padded_data_size - data_size;
+    memset(bytestream, 0, t);
+    bytestream += t;
+
+    pkt->size   = bytestream - bytestream_start;
+    pkt->flags |= AV_PKT_FLAG_KEY;
+    *got_packet = 1;
+
+    return 0;
+}
+
+AVCodec ff_fits_encoder = {
+    .name           = "fits",
+    .long_name      = NULL_IF_CONFIG_SMALL("Flexible Image Transport System"),
+    .type           = AVMEDIA_TYPE_VIDEO,
+    .id             = AV_CODEC_ID_FITS,
+    .priv_data_size = sizeof(FITSContext),
+    .init           = fits_encode_init,
+    .encode2        = fits_encode_frame,
+    .pix_fmts       = (const enum AVPixelFormat[]) { AV_PIX_FMT_RGBA64BE,
+                                                 AV_PIX_FMT_RGB48BE,
+                                                 AV_PIX_FMT_RGBA,
+                                                 AV_PIX_FMT_RGB24,
+                                                 AV_PIX_FMT_GRAY16BE,
+                                                 AV_PIX_FMT_GRAY8,
+                                                 AV_PIX_FMT_NONE },
+};
diff --git a/libavformat/img2enc.c b/libavformat/img2enc.c
index 1297b1a..25283cc 100644
--- a/libavformat/img2enc.c
+++ b/libavformat/img2enc.c
@@ -237,6 +237,7 @@  AVOutputFormat ff_image2_muxer = {
 AVOutputFormat ff_image2pipe_muxer = {
     .name           = "image2pipe",
     .long_name      = NULL_IF_CONFIG_SMALL("piped image2 sequence"),
+    .extensions     = "fits",
     .priv_data_size = sizeof(VideoMuxData),
     .video_codec    = AV_CODEC_ID_MJPEG,
     .write_header   = write_header,