-
Notifications
You must be signed in to change notification settings - Fork 1
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
revisit time-related vocabulary #192
Comments
Another issue with the current usage of
To me, the primary sources of the problem here is that
$ ffmpeg -i cpb-aacip-77-074tnfhr.mp4 2>&1 | grep -i fps
Stream #0:0[0x1](und): Video: h264 (Main) (avc1 / 0x31637661), none(progressive), 480x360, 659 kb/s, SAR 1:1 DAR 4:3, 29.97 fps, 29.97 tbr, 30k tbn (default)
$ python -c 'import cv2; print(cv2.VideoCapture("cpb-aacip-77-074tnfhr.mp4").get(cv2.CAP_PROP_FPS))' 2>/dev/null
# (...)
29.97002997002997
$ python -c 'a=29.97; b=29.97002997002997; print(set([abs(int(i * a) - int(i * b)) for i in range(240*60*30) if int(i * a) != int(i * b)]))'
{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13} The second point is less of a problem, and we can probably live with 3-4 frame differences for 1-hr videos it. We can also somewhat mitigate this by forcing rounded-to-hundredths fps (consistently 29.97) in the SDK code (which I already did last week in the develop branch), hoping the devs are lazy enough not to implement their own fps calculation.
In any case, I strong suggest we should drop using @kelleyl , @snewman-aa you guys have plenty experience with video-related CLAMS apps, and I'd like to hear your thoughts on this. |
I see no reason not to drop I think this unit conversion may have been the number one source of bugs in my apps so far, so I definitely wouldn't mind removing it all together, though the conversion utils are still convenient for development. |
Today I Learned: frame count numbers are not unambiguous. There is The second proposal works better for videos, but I don't think there's a universal way to convert frame nums to audio-compatible timecode in reliable way. |
The more I'm thinking about this, the more I'm leaning toward using milli- or microsecond as the only timeunit, and make all the time-related values typed as
|
A (small) problem with using microseconds as the only timeunit: 1 microsecond is a millionth of a second, meaning 1 hour is 3,600,000,000 microsecond. Since the maximum "simple" signed int on 32-bit systems is 2^31 - 1 = 2,147,483,648, on these systems simple integer data type only works only up to ~35 min long media (~70 min with unsigned int). This is indeed a very "small" problem considering;
|
We (@jarumihooi @owencking and me) talked about this issue in yesterday's meeting, and we (in general) agreed to use either integer milli or micro-seconds as THE standard time unit to appear in all MMIF output of future versions of CLAMS apps. I just wonder if that decision would cause any trouble in terms of GBH's downstream processing workflow (pbcore conversion, relational db operations, etc), so summoning @mrharpo @afred @foo4thought . |
Another "feature" that can be helpful for human readers of MMIF is having |
At the moment, it seems we've reached a consensus of going forward with all gold (and mmif??) datasets using the ISO 8601 standard I forgot to add the reasoning about audio not having frames to the aapb-annotations/readme.md but otherwise I think this could be closed until further business needs require revisiting this or more precision digits. |
Note, while a decision was made, raws and golds currently do not follow the convention yet, as that requires changing both tools and dataset for compatibility. |
I'll keep this open until we
For data in the |
Currently do all the outputs from apps to mmif record this in the ISO Time Format? Into the future, if its an in-house anno tool, it should use this format where possible. if an out-of-house annotation tool or it is not possible, then As for the previous projects, reconverting the time format is likely a lot of work and may cause troubles with apps or currently working code to fail. |
Ideally, all the "currently" working evaluation code should pull the gold data by permalinks. So unless we re-write That said, obviously none of current clams apps uses ISO format for time notation, since officially, only valid time units are seconds, ms, frame numbers (https://mmif.clams.ai/vocabulary/Region/v1/), and the specification hasn't been updated based on the above discussion. What I'm not very sure about implementing this is the Another aspect of the problem we need to consider is that MMIF is NOT INTENDED FOR HUMAN READERS. For human readers, we have mmif-viz. Hence, if we need some "human-friendly feature" such as ISO time notation, probably mmif-viz is the right place to implement it, not within So going back to the other questions,
I don't think so. App should all expect integer-valued
Totally agree that any future in-house annotation environment (for human annotators) must use ISO time format. Not quite sure about the second part, though. Again, gold files are mostly for machine consumption. We can add ISO format as human-friendly columns next to machine-friendly start/end columns in the "gold" format. And writing that conversion (just once) in the
I guess this will completely depends on how the annotation tool, and "upload" steps are implemented. If you're referring existing raw files from older project, I wouldn't bother re-formatting them.
Now I'm not sure in this part you were referring to "raw" or "gold" files of older project. However, it will be worth re-formatting "gold" files because we will probably re-using them in the future over and over (old versions of "gold" files are still accessible via git history and github permalinks, so no worry about losing anything). So having those files up-to-date with what we decide should be worth the effort, as long as the gold data can be used for any purpose (evaluation, mostly). |
Just for future reference, all "gold" files for older annotation project (2022-23 season) are now updated to use the ISO time format via clamsproject/aapb-annotations#77. |
Currently, we allow
seconds
,milliseconds
, andframes
as possible values oftimeUnit
metadata property. We might want to re-visit this metadata property so thatISO8601
or othertimecode
formats.timeUnit
in the JSON map (by enumerating all possible values explicitly in the vocabulary, instead of having a genericstring
typeThe text was updated successfully, but these errors were encountered: