On Tue, Jul 4, 2017 at 2:51 PM, Nicolas George <geo...@nsup.org> wrote:
> 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 <paraschadh...@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.
>
Okay, i will replace it with a strict signature which consists of SIMPLE in
1-6 bytes, spaces in 7-8 bytes, = followed by space in 9-10 bytes and
spaces followed by a single character T/F in byte 30. This would be
sufficient i guess.
>
> > +
> > +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.
>
Okay, will do this.
>
> > + 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.
>
okay
>
> > + 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.
>
Yes, will do it.
>
> > + 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.
>
okay
>
> > + return ret;
> > +
> > + if (avio_feof(s->pb))
> > + return AVERROR_EOF;
> > +
>
> > + pos = avio_tell(s->pb);
>
> I think you could just "pos += size".
>
okay
>
> > + 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.
>
yes, 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.
>
There is no global header.
Basically FITS files can have multiple images. To support them, i had added
a demuxer. Each image consists of a header part and a data part. The header
part of image begins with a SIMPLE keyword or XTENSION = IMAGE keyword,
followed by some mandatory keywords, followed by some optional keywords and
ends with the END keyword. The size of header must be a multiple of 2880
bytes. If it is not, then it is padded with spaces at the end to make it a
multiple of 2880. After this comes the data part. It's size is determined
by the NAXISN fields of the header part. Each NAXISN corresponds to one
dimension of the data and there can be maximum 999 dimensions. So,
basically the product of all NAXISN keywords give the data size (there are
other keywords too which may affect this like PCOUNT, GCOUNT, GROUPS
keyword ). Similar to the header part, the size of data part must also be a
multiple of 2880. It is padded with 0 to make it multiple of 2880.
So, what i am doing here is I am calculating the header size (which is no
of bytes read til the END keyword, including END) and data size (calculated
using information from header) in the find_size function. If the data
contains image (i.e. if SIMPLE or IMAGE XTENSION is present) then i am
allocating a packet of appropriate size and sending it to decoder.
Otherwise i am simply skipping over the data to read the next header (if
any).
Now, i know i am reading the same header twice once in demuxer and then in
decoder. But i guess, there is no other way around. In order to calculate
the size of header i must read the header till the END keyword. In order to
calculate the data size, i must read the NAXIS keywords in the header.
Regarding separation of separation of data that must be handled by demuxer
and decoder, FITS is a generat transport format too. Besides images, it
also supports BInary Tables, ASCII tables which are used to store the data.
Along with it there is something known as Random groups structure. In order
to find the size of their data portions, i must read some other keywords
like GROUP, PCOUNT, GCOUNT etc. So, i must also read them to skip the data
portions of these structures (as there my be an image after them). Besides
this, in decoder i have to read some other keywords like DATAMIN, DATAMAX,
BSCALE, BZERO etc. These are used to manipulate the image to show them
correctly and these keywords can occur in any order within the header
before the END keyword.
One way, i initially thought to solve this problem was to read the header
in the demuxer only, extract all the required keywords and their values and
then send the information to the decoder for processing along with the data
portions. But i could not find a way to do this.
So, now the whole problem is clear. Please suggest a way (if there is any)
to read the header only once and avoid the use of avio_seek.
>
> > +
> > + 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.
>
So, i should send the corrupted packet to the decoder ? But i have added
checks in the decoder that if the packet contains any missing mandatory
keyword it will return AVERROR_INVALIDDATA. If the data portion is also
incomplete, then also it will return AVERROR_INVALIDDATA. So, should i
really send a partial packet to the decoder ?
>
> > +
> > + 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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
