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

SMPTE timecode parsed incorrectly (23.976 FPS) #240

Open
Sebastian-Brud-Tectonic opened this issue Mar 3, 2023 · 1 comment
Open

SMPTE timecode parsed incorrectly (23.976 FPS) #240

Sebastian-Brud-Tectonic opened this issue Mar 3, 2023 · 1 comment

Comments

@Sebastian-Brud-Tectonic
Copy link

Function in doc.js called extractTimeAndTickRate might contain a bug that seems like an impediment in development of one of our tools. The line

} else if ((m = CLOCK_TIME_FRAMES_RE.exec(str)) !== null) {
to be precise

The issues we currently face are:

  1. parsing begin and end timecodes are regexp-based

    • it is cool and nice approach for resilient timecode picker, yet proved to not cover corner-cases (nonDropFrame encoding, etc.)
    • it ignores ttp:timeBase parameter's value (which is not a best practice)
  2. timecode parser does not consider SMPTE cases with dropFrame encoding

    • it ignores attribute ttp:dropMode - not necessary to pass each Regex test
    • it does not cover Drop-Frame encoded timecodes (like library smpte-timecode does)

Few solutions I might think of (that would not affect the library itself too much):

  1. allow passing custom timecode parser to imsc.fromXML function. Implementing Dependency Injection here is a nice way to get rid of multiple edge cases you would not want to cover and maintain by yourselves

  2. implement standard-compliant parser that will cover all cases (Drop/Non-Drop Frame encoding, 2:3 pull-down on 23.976, etc.) - includes lot of effort implementing and preparing proper tests

  3. make fromXML something more like a declarative pipeline ( where you can provide callbacks that can override parsing process steps - see: https://rxjs.dev/guide/higher-order-observables ) that processes the sax stream

  4. become dependent on smpte-timecode library - worst solution IMHO. I like how you rely on sax parser, yet personally I prefer to have more control over dependencies I include into my codebase

@nigelmegitt
Copy link
Contributor

The only permitted ttp:timeBase value in IMSC is media. As a reference IMSC player, it makes sense that it ignores ttp:dropMode since that only applies when ttp:timeBase="smpte".

Having said that, it's an interesting idea to refactor to allow injection of additional out-of-spec functionality.

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

No branches or pull requests

2 participants