Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve gopro support #473

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

vdeconinck
Copy link

tl;dr

GoPro files (at least on Hero 8, 9 and 12) cause crashes of IsoViewer.
I tracked down the issues to parsing typos or misconfigurations and I propose this PR which combines the different fixes.
The text below lists the issues, root cause of each problem and fix applied. Each one corresponds to one commit in the PR.

The big drawback of this is that I had to disable the test "TimeCodeBoxTest" (more details below). If there is a way to adapt that test, I'd be glad to implement it...

Here are the details, please take a coffee ;-)

0) Support blanks in project path.

This is totally unrelated but all tests failed on my machine because my projects reside in a path with blanks.

Problem: Many tests use getLocation().getFile() in which blanks are encoded as %20, resulting in FileNotFoundException failures.

Fix: Call URLDecode on the resulting String to restore the blanks.

Now for the parsing issues:

1) getContentSize() returns the size required to export the contents, not the size read when importing the contents.

For some rare items, the value is different because the source file includes useless "stuffing". This was the case for AudioSpecificConfig, where GoPro files contain 5 bytes for the bitmap while 4 would be sufficient.
In itself, it's not a big deal: when calling getSize(), which is based on getContentSize(), you get the required size, not the parsed one.

Problem: IsoParser stops parsing only when the number of bytes read equals the full size of the contents (computed by using getSize()).
As getSize() of AudioSpecificConfig returns one byte less than actual the size in the file, the sum of contents parsed is wrongly considered one byte less than the file size,
So the parser tries to read one more byte after the end of the file and fails with an EOFException.

Fix: in AudioSpecificConfig, change getContentSize() so that, if the bitmap was parsed from a source and that size was larger than the minimum required, the size in the source is returned.
Note1: this fix is safe because getContents() allocate the bitmap based on getContentSize(), so rewriting a file previously parsed including stuffing will result in the newly written file also including stuffing.
Note2: this change could potentially be applied everywhere and not only in this specific case, but I'm not in a position to decide such a refactoring :-)

2) GoPro files make heavy usage of "timecode" information, and 'tmcd' parsing is not configured correctly.

Herited from Quicktime, several parts of an MPEG4 file can relate to TimeCode, See https://developer.apple.com/documentation/quicktime-file-format/timecode_media
Namely, in GoPro files, one can find 'tmcd':

a) under moov\trak[0]\tref or moov\trak[1]\tref, as follows:

     00 00 00 14 74 72 65 66 00 00 00 0c 74 6d 63 64
     00 00 00 03

which must be interpreted as a timecode "track reference type" on those tracks, referencing the third track containing timecode:

   00 00 00 14 74 72 65 66
=> size = 0x14  t  r  e  f
   00 00 00 0c 74 6d 63 64
=> size = 0x0c  t  m  c  d
   00 00 00 03
=> trackIds = [3]

Problem: the 'tmcd' box being linked to TimeCodeBox in isoparser2-default.properties, that box parsing gets triggered instead of a TrackReferenceTypeBox, and it crashes because there aren't enough bytes.

b) under moov\trak[2]\mdia\hdlr, as follows:

   00 00 00 2c 68 64 6c 72 00 00 00 00 6d 68 6c 72
   74 6d 63 64 00 00 00 00 00 00 00 00 00 00 00 00
   0b 47 6f 50 72 6f 20 54 43 44 20 20

which is correctly interpreted as an unknown 'tmcd' handler inside a handler box

   00 00 00 2c 68 64 6c 72 00 00 00 00
=> size = 0x2c  h  d  l  r (flags = 0)
   6d 68 6c 72
=>  m  h  l  r (?)
   74 6d 63 64
=>  t  m  c  d (= handler type)
   00 00 00 00 00 00 00 00 00 00 00 00
=> a = 0       b = 0       c = 0
   0b 47 6f 50 72 6f 20 54 43 44 20 20
=>l=11 G  o  P  r  o     T  C  D       (name="GoPro TDC  ")

Problem: None.

c) under moov\trak[2]\mdia\minf\gmhd, as follows:

   00 00 00 32 74 6d 63 64 00 00 00 2a 74 63 6d 69
   00 00 00 01 00 15 00 00 00 0a 00 00 00 00 00 00
   00 00 ff ff ff ff ff ff 09 48 65 6c 76 65 74 69
   63 61

which must be interpreted as a tmcd containing a tcmi - https://developer.apple.com/documentation/quicktime-file-format/timecode_media_information_atom

   00 00 00 32 74 6d 63 64
=> size = 0x32  t  m  c  d
   00 00 00 2a 74 63 6d 69
=> size = 0x2a  t  c  m  i
   00
=> version = 0
   00 00 01
=> flags=01
   00 15
=> font=0x15
   00 00
=> face=0x00 (plain)
   00 0a
=> size=0x0a
   00 00
=> (reserved)
   00 00 00 00 00 00
=> color = (00,00,00)
   ff ff ff ff ff ff
=> bgcolor = (ff,ff,ff)
   09 48 65 6c 76 65 74 69 63 61
=> l=9 H  e  l  v  e  t  i  c  a (font name, Pascal style)

Problem: the 'tmcd' triggers the parsing by TimeCodeBox and, while it does not crash, the fields displayed are meaningless because data is not in that format

d) under moov\trak[2]\mdia\minf\stbl\stsd\tmcd, as follows:

   00 00 00 22 74 6d 63 64 00 00 00 00 00 00 00 01 
   00 00 00 00 00 00 00 02 00 01 5f 90 00 00 0b bb 
   1d 00

which is (almost) correctly interpreted as an Apple TimeCodeBox:

   00 00 00 22 74 6d 63 64
=> size = 0x22  t  m  c  d
   00 00 00 00 00 00
=> (reserved)
   00 01 
=> dataReferenceIndex=1
   00 00 00 00 
=> (reserved)
   00 00 00 02 
=> Timecode flags = 0x2 (Indicates the timecode wraps after 24 hours)
   00 01 5f 90
=> Timescale = 15F90h = 90000d
   00 00 0b bb
=> Frame duration = BBBh = 3003d
   1d
=> Number of frames = 1Dh = 29d 
   00
=> (reserved)
=> (no additional source reference information)

Problem: the last (reserved) field is actually on 1 byte, not 3 (According to Apple Quicktime docs, to ffmpeg source and to GoPro samples). See commit message for more details and references.

Fixes:
Case b) is OK
Case d) is an easy fix (read and write 1 byte instead of 3)
But for case a) and c), the source of the issue comes from the fact that they are all interpreted as an Apple TimeCodeBox while they are not.
This is due to the following mapping in isoparser2-default.properties:
tmcd=org.mp4parser.boxes.apple.TimeCodeBox
which must be edited to add the stsd- suffix, as follows:
stsd-tmcd=org.mp4parser.boxes.apple.TimeCodeBox

Unfortunately, the TimeCodeBoxTest now fails miserably, because the TimeCodeBox is parsed as an UnknownBox when not inside an stsd.
Modifying the hex string in the test class to add an enclosing stsd is not sufficient, because the BoxWriteReadBase.roundtrip() wants to write and read the single box under test, and I have no idea how to pretend it's inside a stsd...
The test was thus disabled (renamed) for the time being...

Once TimeCodeBox is limited to tmcd inside stsd, no crash happens anymore, but the 'tmcd' as track reference and the 'tmcd' as a container for 'tcmi' are considered Unknown.
case a): the following line was added to isoparser2-default.properties: tref-tmcd=org.mp4parser.boxes.iso14496.part12.TrackReferenceTypeBox(type)
case c): two classes TimeCodeContainerBox and TimeCodeMediaInformationBox are now implemented and associated in the isoparser2-default.properties file to gmhd-tmcd and tcmi, respectively.

3) Additionally, the current TimecodeBox implementation uses a field called "long flags;"

Problem: IsoViewer introspects fields and matches any "flags" field by name, casting it to an Int (in org/mp4parser/isoviewer/views/BoxPane.kt lines 137-138).
This causes a crash (java.lang.ClassCastException: class java.lang.Long cannot be cast to class java.lang.Integer)
Fix: solved by renaming 'flags' to 'lFlags'

Wow, that was long.

Sorry for that.
But I really needed to analyze and try to fix corrupt MPEG4 files, and your work clearly is the best open-source Java MPEG4 parser.
So although I see there was not much recent activity on it, I really hope you will consider merging this PR to hopefully make mp4parser even better :-)

Have a nice day,

Vincent

…hat getSize() returns the actual size in the opened file, even if it is not packed optimally (e.g. GoPro)
Note This adheres with the following definition
(base fields in https://developer.apple.com/documentation/quicktime-file-format/sample_description_atom )

  Sample description size A 32-bit integer indicating the number of bytes in the sample description.
  Data format A 32-bit integer indicating the format of the stored data. This depends on the media type, but is usually either the compression format or the media type.
  Reserved Six bytes that must be set to 0.
  Data reference index A 16-bit integer that contains the index of the data reference to use to retrieve data associated with samples that use this sample description. Data references are stored in data reference atoms.
(followed by fields in in https://developer.apple.com/documentation/quicktime-file-format/timecode_sample_description )
  Reserved A 32-bit integer that is reserved for future use.
  Flags A 32-bit integer containing flags that identify some timecode characteristics.
  Time scale A 32-bit integer that specifies the time scale for interpreting the frame duration field.
  Frame duration A 32-bit integer that indicates how long each frame lasts in real time.
  Number of frames An 8-bit integer that contains the number of frames per second for the timecode format.
  Reserved An 8-bit quantity. !!!!!!!!
  Source reference A user data atom containing information about the source tape.

See also write implementation in ffmpeg source at https://ffmpeg.org/doxygen/3.3/movenc_8c_source.html#l02057 which is minimal but compatible in terms of sizes.
  avio_wb32(pb, 0);                        size
  ffio_wfourcc(pb, tmcd);                Data format
  avio_wb32(pb, 0);                        Reserved
  avio_wb32(pb, 1);                        Data reference index
  avio_wb32(pb, 0);                        Flags
  avio_wb32(pb, track-timecode_flags);    Flags (timecode)
  avio_wb32(pb, track-timescale);         Timescale
  avio_wb32(pb, frame_duration);           Frame duration
  avio_w8(pb, nb_frames);                  Number of frames
  avio_w8(pb, 0);                          Reserved !!!!!!!!
… recognized by name in IsoViewer and assumed to be an Integer. See org/mp4parser/isoviewer/views/BoxPane.kt:137-138 )
Unfortunately, TimeCodeBoxTest only supports stand-alone TimeCodeBox => disabled test for now :-(
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant