[FFmpeg-devel] [PATCH] Add FITS Demuxer
Nicolas George
george at nsup.org
Tue Jul 4 12:21:12 EEST 2017
Le quartidi 14 messidor, an CCXXV, Paras Chadha a écrit :
> Filled buf with 0 to prevent overfow
> Also added checks for integer overflow
>
> Signed-off-by: Paras Chadha <paraschadha18 at gmail.com>
> ---
> libavformat/Makefile | 1 +
> libavformat/allformats.c | 1 +
> libavformat/fitsdec.c | 224 +++++++++++++++++++++++++++++++++++++++++++++++
> libavformat/version.h | 2 +-
> 4 files changed, 227 insertions(+), 1 deletion(-)
> create mode 100644 libavformat/fitsdec.c
>
> diff --git a/libavformat/Makefile b/libavformat/Makefile
> index 80aeed2..1d43c05 100644
> --- a/libavformat/Makefile
> +++ b/libavformat/Makefile
> @@ -164,6 +164,7 @@ OBJS-$(CONFIG_FFMETADATA_MUXER) += ffmetaenc.o
> OBJS-$(CONFIG_FIFO_MUXER) += fifo.o
> OBJS-$(CONFIG_FILMSTRIP_DEMUXER) += filmstripdec.o
> OBJS-$(CONFIG_FILMSTRIP_MUXER) += filmstripenc.o
> +OBJS-$(CONFIG_FITS_DEMUXER) += fitsdec.o
> OBJS-$(CONFIG_FLAC_DEMUXER) += flacdec.o rawdec.o \
> flac_picture.o \
> oggparsevorbis.o \
> diff --git a/libavformat/allformats.c b/libavformat/allformats.c
> index a0e2fb8..74a2e15 100644
> --- a/libavformat/allformats.c
> +++ b/libavformat/allformats.c
> @@ -121,6 +121,7 @@ static void register_all(void)
> REGISTER_MUXDEMUX(FFMETADATA, ffmetadata);
> REGISTER_MUXER (FIFO, fifo);
> REGISTER_MUXDEMUX(FILMSTRIP, filmstrip);
> + REGISTER_DEMUXER (FITS, fits);
> REGISTER_MUXDEMUX(FLAC, flac);
> REGISTER_DEMUXER (FLIC, flic);
> REGISTER_MUXDEMUX(FLV, flv);
> diff --git a/libavformat/fitsdec.c b/libavformat/fitsdec.c
> new file mode 100644
> index 0000000..3670fbf
> --- /dev/null
> +++ b/libavformat/fitsdec.c
> @@ -0,0 +1,224 @@
> +/*
> + * FITS demuxer
> + * 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 demuxer.
> + */
> +
> +#include "libavutil/intreadwrite.h"
> +#include "internal.h"
> +#include "libavutil/opt.h"
> +
> +typedef struct FITSContext {
> + const AVClass *class;
> + AVRational framerate;
> + int image;
> + int64_t pts;
> +} FITSContext;
> +
> +static int fits_probe(AVProbeData *p)
> +{
> + const uint8_t *b = p->buf;
> +
> + if (AV_RB32(b) == 0x53494d50 && AV_RB16(b+4) == 0x4c45)
> + return AVPROBE_SCORE_MAX - 1;
> + return 0;
> +}
This will match any text file starting with the word "simple" in
uppercase: I think it needs to be much more strict.
> +
> +static int fits_read_header(AVFormatContext *s)
> +{
> + AVStream *st = avformat_new_stream(s, NULL);
> + FITSContext * fits = s->priv_data;
> +
> + if (!st)
> + return AVERROR(ENOMEM);
> +
> + st->codecpar->codec_type = AVMEDIA_TYPE_VIDEO;
> + st->codecpar->codec_id = AV_CODEC_ID_FITS;
> +
> + avpriv_set_pts_info(st, 64, fits->framerate.den, fits->framerate.num);
> + fits->pts = 0;
> + return 0;
> +}
> +
> +static int64_t find_size(AVIOContext * pb, FITSContext * fits)
> +{
> + int bitpix, naxis, dim_no, i, naxisn[999], groups=0;
> + int64_t header_size = 0, data_size=0, ret, pcount=0, gcount=1, d;
> + char buf[81], c;
> +
> + memset(buf, 0, 81);
Since you read all 80 octets, you can just set the last one to 0.
Alternatively, "char buf[81] = { 0 }" should be equivalent.
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
Here and everywhere: please do not discard error codes: ret < 0 must be
returned as is, and only 0 <= ret < 80 should generate EOF.
> + if (!strncmp(buf, "SIMPLE", 6) || !strncmp(buf, "XTENSION= 'IMAGE", 16)) {
> + fits->image = 1;
> + } else {
> + fits->image = 0;
> + }
> + header_size += 80;
> +
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
> + if (sscanf(buf, "BITPIX = %d", &bitpix) != 1)
> + return AVERROR_INVALIDDATA;
> + header_size += 80;
> +
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
> + if (sscanf(buf, "NAXIS = %d", &naxis) != 1)
> + return AVERROR_INVALIDDATA;
> + header_size += 80;
> +
> + for (i = 0; i < naxis; i++) {
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
> + if (sscanf(buf, "NAXIS%d = %d", &dim_no, &naxisn[i]) != 2)
> + return AVERROR_INVALIDDATA;
> + if (dim_no != i+1)
> + return AVERROR_INVALIDDATA;
> + header_size += 80;
> + }
> +
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
> + header_size += 80;
> +
> + while (strncmp(buf, "END", 3)) {
> + if (sscanf(buf, "PCOUNT = %ld", &d) == 1) {
> + pcount = d;
> + } else if (sscanf(buf, "GCOUNT = %ld", &d) == 1) {
> + gcount = d;
> + } else if (sscanf(buf, "GROUPS = %c", &c) == 1) {
> + if (c == 'T') {
> + groups = 1;
> + }
> + }
> +
> + if ((ret = avio_read(pb, buf, 80)) != 80)
> + return AVERROR_EOF;
> + header_size += 80;
> + }
> +
> + header_size = ceil(header_size/2880.0)*2880;
As said before, use integer arithmetic. And if nothing prevents it, make
header_size unsigned.
> + if (header_size < 0)
> + return AVERROR_INVALIDDATA;
> +
> + if (groups) {
> + fits->image = 0;
> + if (naxis > 1)
> + data_size = 1;
> + for (i = 1; i < naxis; i++) {
> + data_size *= naxisn[i];
> + if (data_size < 0)
> + return AVERROR_INVALIDDATA;
> + }
> + } else if (naxis) {
> + data_size = 1;
> + for (i = 0; i < naxis; i++) {
> + data_size *= naxisn[i];
> + if (data_size < 0)
> + return AVERROR_INVALIDDATA;
> + }
> + } else {
> + fits->image = 0;
> + }
> +
> + data_size += pcount;
> + data_size *= (abs(bitpix) >> 3) * gcount;
> +
> + if (data_size < 0)
> + return AVERROR_INVALIDDATA;
> +
> + if (!data_size) {
> + fits->image = 0;
> + } else {
> + data_size = ceil(data_size/2880.0)*2880;
> + if (data_size < 0)
> + return AVERROR_INVALIDDATA;
> + }
> +
> + if(header_size + data_size < 0)
> + return AVERROR_INVALIDDATA;
> +
> + return header_size + data_size;
> +}
> +
> +static int fits_read_packet(AVFormatContext *s, AVPacket *pkt)
> +{
> + int64_t size=0, pos, ret;
> + FITSContext * fits = s->priv_data;
> +
> + fits->image = 0;
> + pos = avio_tell(s->pb);
> + while (!fits->image) {
> + if ((ret = avio_seek(s->pb, pos+size, SEEK_SET)) < 0)
Note that we have avio_skip() for that.
> + return ret;
> +
> + if (avio_feof(s->pb))
> + return AVERROR_EOF;
> +
> + pos = avio_tell(s->pb);
I think you could just "pos += size".
> + if ((size = find_size(s->pb, fits)) < 0)
> + return size;
> + }
> +
> + if ((ret = avio_seek(s->pb, pos, SEEK_SET)) < 0)
> + return ret;
I really REALLY do not like this.
For one, it makes seeking mandatory for no good reason. But that is the
least of it.
The worst part is that the same chunk of data is both decoded by this
demuxer and read into the packet payload to be decoded by the decoder. A
very visible consequence of that is the function find_size() above that
shares a lot of logic with the read_header() function in the decoder,
with a different interface to access the data (avio instead of a full
buffer).
I was about to suggest you find a way of sharing that logic, but now I
realize it is only the tip of the problem.
Is there a standard audio/video file format that supports FITS video
streams? Matroska? MPEG-TS?
If the answer is yes, then we must be compatible, and that ends the
question. But I think the answer is no.
If the answer is no, then we, i.e. you, must decide what is considered
part of the format and must be handled in the demuxer, and what is
considered part of the packet payload and must be handled by the
decoder.
This is quite easy, but it must be done properly, because other projects
may imitate us.
What I suggest is this: take the dump of a typical FITS file with three
images and annotate it with "this is the global header", "this is the
header of the first packet", "this is the payload of the first packet",
"this is the header of the second packet", etc. Then post it here for
review.
Note that if there is no global header, then this demuxer should
probably be just a parser for the image2pipe demuxer.
> +
> + ret = av_get_packet(s->pb, pkt, size);
> + if (ret != size) {
> + if (ret > 0) av_packet_unref(pkt);
> + return AVERROR(EIO);
> + }
I think the correct handling would be a packet with AV_PKT_FLAG_CORRUPT.
> +
> + pkt->stream_index = 0;
> + pkt->flags |= AV_PKT_FLAG_KEY;
> + pkt->pos = pos;
> + pkt->size = size;
> + pkt->pts = fits->pts;
> + fits->pts++;
> +
> + return size;
> +}
> +
> +static const AVOption fits_options[] = {
> + { "framerate", "set the framerate", offsetof(FITSContext, framerate), AV_OPT_TYPE_VIDEO_RATE, {.str = "1"}, 0, INT_MAX, AV_OPT_FLAG_DECODING_PARAM},
> + { NULL },
> +};
> +
> +static const AVClass fits_demuxer_class = {
> + .class_name = "FITS demuxer",
> + .item_name = av_default_item_name,
> + .option = fits_options,
> + .version = LIBAVUTIL_VERSION_INT,
> +};
> +
> +AVInputFormat ff_fits_demuxer = {
> + .name = "fits",
> + .long_name = NULL_IF_CONFIG_SMALL("Flexible Image Transport System"),
> + .priv_data_size = sizeof(FITSContext),
> + .read_probe = fits_probe,
> + .read_header = fits_read_header,
> + .read_packet = fits_read_packet,
> + .priv_class = &fits_demuxer_class,
> + .raw_codec_id = AV_CODEC_ID_FITS,
> +};
> diff --git a/libavformat/version.h b/libavformat/version.h
> index 44c16ac..48b81f2 100644
> --- a/libavformat/version.h
> +++ b/libavformat/version.h
> @@ -32,7 +32,7 @@
> // Major bumping may affect Ticket5467, 5421, 5451(compatibility with Chromium)
> // Also please add any ticket numbers that you believe might be affected here
> #define LIBAVFORMAT_VERSION_MAJOR 57
> -#define LIBAVFORMAT_VERSION_MINOR 75
> +#define LIBAVFORMAT_VERSION_MINOR 76
> #define LIBAVFORMAT_VERSION_MICRO 100
>
> #define LIBAVFORMAT_VERSION_INT AV_VERSION_INT(LIBAVFORMAT_VERSION_MAJOR, \
Regards,
--
Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170704/fc9e3821/attachment.sig>
More information about the ffmpeg-devel
mailing list