[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Nicolas George]

Le decadi 30 messidor, an CCXXV, Rostislav Pehlivanov a écrit :
> Its a crappy format, no reason to blame anyone else but the format.
> We have plenty of crappy formats which have no clear separation between
> packets so demuxers have to give not-entirely-demuxed packets to the
> decoder which also has to skip past junk. Its both wrong and its not,
> because it isn't defined anywhere and the packets packed in the file were
> never meant to go anywhere but the format they were packed in.

I mostly agree, but I would say that exactly the same applies to many
image formats, especially old ones, including some amongst GIF, Sun
Raster, XPM / XBM, XFace, Targa, etc.

Do you disagree?

> I think the image2 demuxer shouldn't handle this type of junk/useless data
> filtering and would rather see a separate demuxer like the current patch
> which deals with crap.

I am somewhat confused by your statement, because in my mind, this is
exactly the purpose and task of the image2(pipe) demuxers.

The image2pipe demuxer reads the input stream, calls a parser to find in
it the size of the images and returns them as individual packets. Note
that the parsers are not part of the image2(pipe) demuxers, they are
separate entities of their own, each specific to one image format.

I do not know what the image2pipe demuxer would be good for, if not for
that. Or did I miss something?

Still, I do not insist that it is done with the image2(pipe) demuxer. It
only seems the best solution according to what was said about the format
until now.

What I do insist on, is this:

Look at the find_size() function in this patch:
https://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213076.html

Then look at the fits_read_header() in this patch:
https://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213061.html

They are the same. One works on an AVIOContext using read operations,
the other works on a buffer using pointer arithmetic, but the logic is
the same. And it is not a trivial logic, here, we are talking about
dozens of lines of code.

This is not good design, this is not good for maintenance: if a bug
needs fixing, it will need fixing consistently in both implementations;
if a feature is added, it will require the same.

Good design and maintenance require a single implementation.

This is especially true for an obscure format like FITS, where
maintenance workforce is scarce.

And this is especially true for a GSoC project, where the student is
supposed to be learning good practices.

I think the best and simplest way of achieving that is to have both the
parser and the decoder call the same function. And I think the simplest
way of implementing it is to make a parser and rely on image2pipe.

Because with minimal changes to fits_read_header(), the parser would
look just slightly more complex than that:

static int fitsparse(AVCodecParserContext *s, AVCodecContext *avctx,
                      const uint8_t **poutbuf, int *poutbuf_size,
                      const uint8_t *buf, int buf_size)
{
    fits_read_header(buf, buf_size, &size);
    return size;
}

So as it stands now, I will oppose any patch that duplicates the
header-reading logic, and I think anybody should do the same, since it
is everybody's responsibility to uphold the high standards of code
quality in our project.

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/20170719/554d53e6/attachment.sig>