[FFmpeg-devel] [PATCH] Add FITS Demuxer

[Nicolas George]

Le primidi 1er thermidor, an CCXXV, Reimar Döffinger a écrit :
> I am a bit unclear on the details, but my memory:
> Well, they work better (only?) if the images are completely independent.

> I am not sure this is entirely the case here, for example the first
> image would have a different header it looks like from the muxer
> patch.

According to this mail from Paras Chadha:
https://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213180.html
there is no global header. A first frame that is not identical to the
next one would count as a global header.

I hope Paras Chadha can tell us more about this issue.

> There is also the question: how would the encoding work?
> Because if encoding will be using a muxer, having things symmetric by
> having a demuxer is an advantage.

If it is an image format (without global headers), it should work just
the same as the other image formats, nothing more: the image2pipe muxer
concatenates the packets.

> I don't think we have the right frameworks for that. Other cases like
> the MPEG video format parsers and decoders also duplicate that kind of
> code.

If we do not have the right framework to avoid code duplication, then we
need to invent it. What you say here only means that sub-standard code
have been accepted in the past, and that should not count as a
precedent.

Also, MPEG is a bit more important than FITS, so rushing things for it
may have been acceptable.

Still, in this particular case, I have reviewed the patches and I know
the code enough to say that merging both functions would be quite easy
even without a framework.

> There might be some ways though, I am mostly thinking of an iterative
> function that is called with a pointer to each 80 byte header buffer
> and updates a state structure with the info it extracts from there.
[1]

Yes, exactly, that is one of the possibilities.

> Would still get messy if the decoder needs quite different data than
> the demuxer.
> I have not yet checked how similar these functions actually are.

I have looked at the code (enough to make a full review of a previous
version), and I can say it is not the case. The information required by
the demuxer is more or less a subset of the information required by the
decoder.

I say more or less because I just saw something fishy in the code:

+    data_size += pcount;
+    data_size *= (abs(bitpix) >> 3) * gcount;

Paras Chadha, can you tell us why this is counted in the data size in
the demuxer and not in the decoder?

> Parsers work best when parsing requires very limited context.
> My impression is that there is no fixed maximum header size, which can
> easily make things really painful in a parser.

Urgh, I thought the framework was better than that. I concede that
turning the existing code into a parser would be more work than I 

Nonetheless, with the coding organization you suggested above (see [1]),
adding buf[80] to the parsing structure would do the trick.

But we still need Paras Chadha's knowledge about the possible
differences in the first frame.

> Also considering the complexity: I suspect that the header parsing can
> be simplified a good bit, and maybe some common helper functions
> extracted.

I think so too.

> I wouldn't insist on avoiding code duplication when trying to share it
> might actually result in more complicated code. But it probably needs
> some thinking to figure out what makes sense.

Having looked at the code, I really think it would not lead to more
complicated code.

> Ok, I think it is possible to use the same code for those, for example
> by doing this:

> - in the demuxer (or parser, but that is harder because you cannot
> just read what you need but have to deal with the data as it is
> provided), get data until the end of the header.

> The end of the header detection would be somewhat duplicated, but
> should be fairly trivial.

Or it could go the other way round: code the function to use an AVIO,
and in the decoder create an AVIO that reads from the packet data.

> - Then use a function that takes such a buffer and returns a
> AVDictionary with name/value pairs. The decoder and demuxer can then
> each decide what they want to do with them.

Wait, what?!?

The decoder makes an AVDictionary for the strings metadata that are
returned as is in the frame, but apart from that, both the decoder and
demuxer only use a few numeric fields: just return these fields in a
structure.

> There is one issue at least though: for files with very large
> meta-data/header this would almost double memory usage in the demuxer
> due to having copies in the dictionary.

I would say many of our components have this kind of issue. I have not
checked, but I think cases where a component reads a full block and then
splits it into string metadata is very common and we never cared.

> There is also to me the issue, while both parse the same things, what
> these functions do is not at all the same semantically, at least
> currently.

> The demuxer only parses a select few keywords with well-known format
> and as a result can e.g, use sscanf directly (not that I like using
> sscanf personally).

> The decoder one builts a metadata dictionary and thus needs a lot of
> special-case handling like proper parsing, handling possibly new
> keywords, deciding what to do with garbage (instead of just skipping),
> handling escaping for the values, ...

It does not seem very complicated to me: just enclose the parts that are
needed by the decoder in "if (decoding) { ... }" blocks. Or even, do the
parsing and discard the value in the demuxer, a few extraneous string
comparisons or conversions to numeric are not a problem.

I just looked at the code again, and it seems to me only the
av_dict_set() calls would need to be disabled in the demuxer, the rest
is just populating a structure with numeric values.

> Personally if it was my job to get this in, I would start by
> simplifying at least one of those functions while making sure to have
> proper bounds checking and error handling to see where the complexity
> really lies and how complex the problem actually is compared to how
> the first try solution looks.

Indeed.

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/20170720/2cb9be13/attachment.sig>