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

Subrib parser compatibility #2145

Closed
mice777 opened this issue Dec 2, 2016 · 6 comments
Closed

Subrib parser compatibility #2145

mice777 opened this issue Dec 2, 2016 · 6 comments
Labels

Comments

@mice777
Copy link

mice777 commented Dec 2, 2016

SubripSubtitle.java

Try to feed it with this text file:

1
-1:58:00,300 --> -1:58:03,540
Test

It will not work as it's not expecting negative times, yet I encountered such srt file.

@ojw28
Copy link
Contributor

ojw28 commented Dec 2, 2016

I don't think negative times are SubRip compliant. Please feel free to refer to a source that says something different, if you've found one. What would it mean for a subtitle to occur during negative time? Thanks!

@ojw28 ojw28 added the question label Dec 2, 2016
@mice777
Copy link
Author

mice777 commented Dec 2, 2016

Compliant or not, I used a tool that shifted times to match movie, and it shifted some times to negative range. It played OK on a video player and I didn't care about negative ones.
It could play too in exoplayer by tiny fix:
private static final Pattern SUBRIP_TIMESTAMP = Pattern.compile("(?:(-?\\d+):)?(\\d+):(\\d+),(\\d+)");
(added "-?")

@ojw28
Copy link
Contributor

ojw28 commented Dec 2, 2016

It seems preferable for you to ask whoever writes the tool you're using to fix the bug (i.e. by clipping the output to t=0). Making non-compliant additions to other software as a means to work around the problem is not the best approach, and furthermore, it encourages the creation of more tools that generate non-compliant output.

Unless you can find a reference that says negative timestamps are allowed, I think you should report the issue to whoever created the tool and get a fix on that side.

@mice777
Copy link
Author

mice777 commented Dec 2, 2016

I don't remember the tool. But many players handled it.

You could simply ignore negative timestamps instead of breaking entire parsing. You have already "Skipping invalid index: " in there which is also support for non-compliant files.

Another point of view: Subrip is probably not standardized at form of "RFC" or similar. You you should point to make the parser as permissive as possible. Btw, are negative timecodes really documented somewhere to be prohibited?

@ojw28
Copy link
Contributor

ojw28 commented Dec 2, 2016

I see what you mean now. You're right. To clarify:

  • Whilst we don't want to actively handle the case (i.e. treat it as valid), we don't want to flat out fail playback either. I was thinking this was covered by the "Skipping invalid timing" logic case, but I guess the failure actually occurs in parseTimecode and is not currently handled gracefully. We will update the code to handle this by skipping to be consistent with the other cases.
  • Re what's really allowed. Yes, I'm equally unsure of whether there exists a definitive spec. The closest to authoritative sources I could find were the WebVTT timestamp definition (which I think was based derived from SubRip) and Wikipedia. Both of which appear to not allow negative timecodes.
  • As a general point I disagree with the "parsers should be as permissive as possible" comment. Parsers being overly permissive encourages content production tools to be non-compliant, and you end up in a cycle where a large proportion of content is non-compliant and where it's no longer possible to implement a viable parser just by reading a specification and doing what it says.

@ojw28 ojw28 added bug and removed question labels Dec 2, 2016
@ojw28
Copy link
Contributor

ojw28 commented Dec 2, 2016

Marking as bug; we will resolve by skipping as per first bullet point. Thanks!

ojw28 added a commit that referenced this issue Dec 2, 2016
Issue: #2145

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140868079
ojw28 added a commit that referenced this issue Dec 2, 2016
Issue: #2145

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140868079
@ojw28 ojw28 closed this as completed Dec 2, 2016
@google google locked and limited conversation to collaborators Jun 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants