[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Nicolas George]

Le tridi 3 thermidor, an CCXXV, Paras Chadha a écrit :
> The only difference between first image and rest is that, the first image
> will always contain the keyword, SIMPLE = T as first keyword while all
> other images will have XTENSION = 'IMAGE ' as first keyword.

Then I would say that "SIMPLE = T" counts as a global header, if one
void of information.

And thus, I strongly suggest to have the demuxer remove the "SIMPLE = T"
and "XTENSION = 'IMAGE '" and only return the rest of the block.
Otherwise, this codec would not be really intra-only and that would be
really a problem.

> Also, PCOUNT
> and GCOUNT are mandatory in all image extensions but not in the first
> image. Rest all keywords are same.

In the first image, are they optional or forbidden?

> It is because for image extensions, PCOUNT = 0 and GCOUNT = 1 are mandatory
> in the header according to the standard.

Ok. But I think you neglected to check that this constraint is met by
the file being read.

> In demuxer, i am skipping over these extensions and only sending the images
> to the decoder.

Ok, that is another argument for having a real demuxer.

Let us find the most efficient way of getting rid of the logic
duplication.

I suggest something like this:

* Create libavcodec/fits.h with:

    typedef enum FITSHeaderState {
        STATE_SIMPLE,
        STATE_XTENSION,
        STATE_BITPIX,
        STATE_NAXIS,
        STATE_NAXIS_I,
        STATE_REST,
    } FITSHeaderState;

    typedef struct FITSHeader {
        FITSHeaderState state;
        unsigned axis_index;
        ...
    } FITSHeader;

    int avpriv_fits_header_init(FITSHeader *header, FITSHeaderState state);

    int avpriv_fits_header_parse_line(FITSHeader *header,
                                      uint8_t line[80],
                                      AVDictionary ***metadata);

* In libavcodec/fitsdec.c, rename fits_read_header() into
  avpriv_fits_header_parse_line(), and in the body of the function,
  surround each block that parses a keyword with:

    if (header->state == STATE_...) {
        /* existing code */
        header->state = STATE_next;
    }

  (or even better with a switch/case statement). Signal END by returning
  a non-zero value.
  
  Of course, all local variables need to be moved to the structure.
  (Sorry I made you do the exact opposite earlier, but I had not yet
  seen the demuxer at the time.)

* The actual fits_read_header() becomes very simple:

    avpriv_fits_header_init(&header, STATE_BITPIX);
    do {
        if (ptr8 - end < 80)
            return AVERROR_INVALIDDATA;
        ret = avpriv_fits_header_parse_line(&header, ptr8, &metadata);
        ptr8 += 80;
    } while (!ret);
    if (ret < 0)
        return ret;
    /* validation of the values, fill_data_min_max, etc. almost
       unchanged */

* Move the code for PCOUNT, GCOUNT, GROUPS from the demuxer to
  avpriv_fits_header_parse_line() and add the corresponding fields in
  the structure.

* The find_size() function is simplified the same way as
  avpriv_fits_header_init() with just a loop calling
  avpriv_fits_header_parse_line(), but with avio_read() instead of ptr8.

  I strongly suggest you take the occasion for keeping a copy of all the
  data in memory to avoid the need for seeking later. You can use the
  AVBPrint API for that, for example.

  Also, the demuxer would pass NULL for the metadata pointer, so each
  metadata affectation must be made conditional. It can be factored with
  a helper function dict_set_if_not_null() or something.

If you follow these steps, you get rid of the huge code duplication, and
you also gain a lot of simplification for error checking. For example:

+    if ((ret = avio_read(pb, buf, 80)) != 80)
+        return AVERROR_EOF;

would only exist once (and you can take the time to fix it, because
negative values must be returned as is, not converted to EOF).

Of course, it is only a set of suggestions. Feel free to discuss them or
choose another path if you see fit. But based on my experience, it is
probably the simplest way of getting code that is simple and easy to
maintain.

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/20170721/49a9df02/attachment.sig>