fiff_open fails on a MAG4Health fif file

[schoffelen]

Hi Eric @larsoner can I pick your brain a bit for the following (or otherwise please point me to whomever is more fit to be brainpicked about this). Vladimir @vlitvak reported an issue with reading a MAG4Health OPM fif-file in FieldTrip (which uses the mne-tools/mne-matlab code). Specifically, MNE-python and brainstorm seem to do just fine. The issue is referenced here:

fieldtrip/fieldtrip#2459

I looked into this a bit, and it turns out that the very last tag in the culprit file has a value of '0' in the 'next' field. This causes fiff_open to try and read beyond the end of the fif-file in the corresponding while loop (lines 95-103). The long story short seems to be that (also according to the FIFFV_NEXT_NONE constant, which is defined as -1) the code kind of expects to be able to escape the while loop when tag.next==-1 at the end of a fif-file.
I could confirm that all other fif-files I have lying around close by indeed have a tag.next==-1 at the end of the file.

I could not find online any chatter about the official way in which the end of a fif-file should be formatted, so I was wondering whether the referenced file is fif-invalid in its definition, and whether that should be brought to the attention of the Mag4health folks. Alternatively, I could of course put a small bandaid on the relevant piece of code and then crawl back into my cave to try and forget about the current state of the world.

[larsoner]

TL;DR: PR welcome to fix MNE-MATLAB!

I could not find online any chatter about the official way in which the end of a fif-file should be formatted

There are PDFs but not very discoverable, so I opened mne-tools/fiff-constants#44 to at least get the ball rolling. Ideally we would autogenerate this using CIs but in the meantime it should at least help.

I was wondering whether the referenced file is fif-invalid in its definition

I think the relevant parts are starting on page #8 (12 in the PDF):

name	type	size	description
next	int32_t	4	Location of the next tag in the file. If this value is 0 , the next tag follows sequentially. If the value is -1 the list of tags ends here. Otherwise next indicates tthe byte position of the next tag in the file.

And

2.4 Tag directory

The purpose of the tag directory is to facilitate searching of information
from a FIFF file. Without the directory, the tag list would have to be read
from the beginning until the desired data are located. The data of dir tag,
which dir_pointer tag points to, is an array of dir_entry_t structures (see
Section A.4.3). Programs can read the directory into memory for search-
ing the contents, and then access the data through pointers in the directory.
The tag directory is not mandatory. Programs using FIFF files should be
able to manage files with or without the directory.

So maybe your suggestion:

I could of course put a small bandaid on the relevant piece of code

I think yes if MNE-Python reads something, MNE-MATLAB should ideally read it the same way to the extent possible. MNE-Python emits a warning that it tried to read a tag (probably at the end of the file) but it was invalid, which I think is what I'd expect:

$ mne show_fiff fyna_research_raw.fif
/home/larsoner/python/virtualenvs/base/bin/mne:8: RuntimeWarning: Invalid tag with only 0/16 bytes at position 173802913 in file /home/larsoner/Desktop/fyna_research_raw.fif
  sys.exit(main())
999  = FIFFB_ROOT
    100  = FIFF_FILE_ID (20b ids)  = {'version': 1234, 'machid': ar ... dict len=4
    101  = FIFF_DIR_POINTER (4b >i4)  = [0]
    100  = FIFFB_MEAS
        101  = FIFFB_MEAS_INFO
            212  = FIFF_EXPERIMENTER (13b str)  = FYNA Research ... str len=13
            204  = FIFF_MEAS_DATE (4b >i4)  = [1729855256]
            206  = FIFF_COMMENT/FIFF_DESCRIPTION (18b str)  = 4He OPMs recording ... str len=18
            200  = FIFF_NCHAN (4b >i4)  = [141]
            x141: 203  = FIFF_CH_INFO (96b cis)  = {'scanno': 141, 'logno': 4703, ... dict len=11
            201  = FIFF_SFREQ (4b >f4)  = [1003.21027]
            106  = FIFFB_SUBJECT
                400  = FIFF_SUBJ_ID (4b >i4)  = [999]
                401  = FIFF_SUBJ_FIRST_NAME (6b str)  = Empty  ... str len=6
                403  = FIFF_SUBJ_LAST_NAME (4b str)  = Room ... str len=4
            102  = FIFFB_RAW_DATA
                201  = FIFF_SFREQ (4b >f4)  = [1003.21027]
                x615: 300  = FIFF_DATA_BUFFER (282564b >f4)  = [ 11.434588     2.4555857  -34 ... array size=70641

And yes I think it's also worth telling the MAG4Health folks that ideally the last tag in the file should have a -1 / FIFFV_NEXT_NONE. Or even better would be a tag directory, but understandable if they don't want to (it's more work and more complicated).

and then crawl back into my cave to try and forget about the current state of the world.

This part I'll let you decide but either way I'm glad you came out into the light for a bit 😆

[schoffelen]

Thanks for the feedback @larsoner! Also for the specs doc. If I read it well, there's a 'nil' pointer mentioned on page 7, which I take to be the FIFFV_NEXT_NONE constant? Either way, I created a PR that throws a warning, rather than an error.

[larsoner]

Yes I think thaht's FIFFV_NEXT_NONE