Drop Frame Support for Timecode #2126
Conversation
Update mediaelement from source
Update fork from source
… toggle for long-video to be based on timecode length rather than duration.
…progress tooltip to show HH:MM:SS:FF
Added function to detect drop frame framerate (fractional framerate). Updated calculations for timeCodeToSeconds and secondsToTimeCode to handle fractional frame rates. Updated output to use semi-colon as third delimitter, per SMPTE standards.
Fixed calls to secondsToTimeCode to use showTimecodeFrameCount and FPS user options. Changed method used to detect and toggle long-video CSS class from duration>1 hour. If time format is HH:MM:SS:FF then the duration check will never toggle. Changed detection to be based on length of timecode provided. IF timecode length is > 5 then the long-video CSS class is used.
The long-video tooltip is only long enough to hold 3 timecode element pairs (e.g. HH:MM:SS). Updated the CSS for long-video time-float elements so they can hold the full format (HH:MM:SS:FF). The margin was also updated to maintain position of the tooltip with the mouse location.
src/js/utils/time.js
Outdated
| minutes = Math.floor(time / 60) % 60, | ||
| seconds = Math.floor(time % 60) | ||
| ; | ||
| dropFrames = Math.round(fps * 0.066666), |
There was a problem hiding this comment.
Why 0.0666666? What does that stands for?
|
Thanks for your work on this PR; is people like you that makes this plugin outstanding. This work LGTM but I'll need you to fix the unit tests so I can completely approve this PR and also answer the question posted above. |
|
Or if you are not opposed I can make the changes for you |
|
Thanks @Ron666. The .066666 represents the number of drop frames to drop on the minute marks. Essentially 6%. You need to calc it as a decimal like this for accuracy before turning it into an integer. The conversions are based on pseudocode from: With regards to the tests, if you want to fix them up, that would be fine, my answers to the three failed ones are: 1) edit: caused by truncating vs rounding errors in original test values. 2) I suspect this is caused by the format conversion being incomplete. SMPTE is usually seen in it's four-pair form. 3) For frames, it doesn't make much sense to have fractional (.decimal) frames. In SMPTE while it looks like time, the purpose is simply to give each frame a unique identifier. The dropframe support is to make it align with a realtime clock, but the purpose is just labelling. So the toFixed functions were set to (0). If people wanted MM:SS and MM:SS.sss, that makes sense, so perhaps what I changed hampers that ability? Let me know if you need more info., or if I really need to get my hands dirty on the tests. |
|
On a side note, what's the best way to run the tests in my local environment for MEJS? Looking at the tests themselves, I can probably come up with something a bit more robust, and make sure the drop frame side gets tested properly. |
|
@Ron666 - Regarding the tests: The first test which failed is due to rounding errors when creating the test values. The actual values that come up, are off by a frame due to the original code probably truncating the value instead of rounding. The second test revealed a minor bug where I returned frames instead of seconds (and it would partially affect the third test). The third test, "can show numeric value with decimals when fps is indicated" is no longer valid since I return toFixed(0). We should ask the community perhaps if anyone needs the FF component with .decimals? To keep the feature we could adapt the timeformat option to allow for HH:MM:SS:FF.sss, or simpler yet, add an option for ffNumberOfDecimals, which would just go directly into the toFixed() call. I could use 0 they can use 3, default 0. This choice will determine how the third failed test gets handled. Should we cancel this PR, and I can put up the fixed bug, along with the updated test values? We could then discuss implications/resolution for the .decimal function of timeCodeToSeconds and the third test? |
|
Yeah I agree let's bring the fix that you mention and to run the test use npm install and then n run test |
|
Sorry npm run test |
Conflicts: src/js/features/progress.js src/js/features/time.js src/js/utils/time.js
Some of the timecode values used in the test were based off of truncated decimal values, rather than rounded decimal values.
|
@Ron666 Looks like everything passed. Once I can get npm to run the test in my environment I'll create some more specific dropframe-related tests. That really only leaves the topic regarding FF.sss in the display and whether to leave it off or create an option for it. |
|
I'd say add it as an option and with that this PR will be covered |
|
OK, I will look into it Sunday. |
|
Sounds like a plan. Let me know when you are ready to review this |
|
@Ron666 Not sure how the builds became a conflict, but the final changes I've uploaded add a user option: secondsDecimalLength. If frames are NOT shown, then the seconds value will show the number of decimal places. Default = 0, no max, though 2 or 3 is probably optimal. If frames are shown then the seconds values have 0 decimal places. Again, it makes more sense to support seconds with decimals, not frames. |
|
Thanks for your work on this. I'll review it one more time, fix the issues and merge |
|
PR merged. Thanks for all the work you did on this |
This update provides for some fixes in the progress tooltip, to show full length timecodes, as well as providing the calculations required for supporting drop frame timecodes (e.g. fractional frame-rate videos using NTSC SD standards). Indicates dropframe using semi-colon, e.g. 00:00:00;00. I've had this in MEJS since version 2, but updated it for 3. One note: due to how MEJS sets up features, this particular item requires the player to be re-initialized if changing video between DF and non-DF videos in the same player. MEJS is outstanding so I felt compelled to share this with everyone, if you're interested please review and merge. Enjoy.