[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Paul B Mahol]

On 7/19/17, Nicolas George <george at nsup.org> wrote:
> Le decadi 30 messidor, an CCXXV, Rostislav Pehlivanov a ecrit :
>> 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.

Except when those standards need to be applied to your work.