-
Notifications
You must be signed in to change notification settings - Fork 571
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
vdeconinck
wants to merge
8
commits into
sannies:master
Choose a base branch
from
vdeconinck:improve_gopro_support
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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 :-(
…diaInformationBox for the tcmi it contains
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
which must be interpreted as a timecode "track reference type" on those tracks, referencing the third track containing timecode:
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:
which is correctly interpreted as an unknown 'tmcd' handler inside a handler box
Problem: None.
c) under moov\trak[2]\mdia\minf\gmhd, as follows:
which must be interpreted as a tmcd containing a tcmi - https://developer.apple.com/documentation/quicktime-file-format/timecode_media_information_atom
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:
which is (almost) correctly interpreted as an Apple TimeCodeBox:
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