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.

Sign up for GitHub

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

Jump to bottom

SSA/ASS subtitles - Overlapping start/end times and position tag is not handled #6595

Merged

icbaker merged 10 commits into google:dev-v2 from szaboa:dev-v2-ssa-position

Dec 5, 2019

Contributor

szaboa commented Oct 29, 2019

Pull request for #6320.

szaboa added 2 commits

October 27, 2019 18:54


          Parse and apply position attribute in SSA subtitles


          Adding support for overlapping subtitles

0391e73

googlebot added the cla: yes label

ojw28 requested a review from icbaker

October 29, 2019 22:48

ojw28 assigned icbaker

icbaker requested changes

View reviewed changes

Collaborator

icbaker left a comment

Thanks for the contribution!

The position stuff looks good, just a few small comments.

I've added a larger comment about the general approach to the handling of overlapping start/end times. It's an awkward bit of algorithm/logic to get right (and in the future if we need to implement it a third time we might try and abstract it into a common utility class that handles all the overlapping resolution etc.).

On the subject of testing:
I think it probably makes sense to test at the level of SsaDecoder (rather than directly testing SsaSubtitle for example). It's probably worth adding my "equal start/end times" case as one of your tests :) As well as nested subtitles too:

[3, 7] -> "A"
[4, 5] -> "B"

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java Outdated

+                      i++;
+                    } while (i != endTimeIndex);
+                  }
+                }

Collaborator

icbaker Oct 30, 2019

I could be wrong, but I'm not convinced this correctly handles multiple cues that have the same start or end time.
e.g.

[3, 5] -> "A"
[4, 5] -> "B"

We'll insert the first one, after which we have: cueTimesUs=[3, 5], startTimeIndex=0 & endTimeIndex=1 and cues=[["A"], []].

Then insert the second one: cueTimesUs=[3, 4, 5, 5], startTimeIndex=1 & endTimeIndex=3 and cues=[["A"], ["A", "B"], ["B"], []]

Whereas I think with this input we'd want these lists: cueTimesUs=[3, 4, 5] and cues=[["A"], ["A", "B"], []]

It also took quite a lot of thought for me to follow that through, especially with the multiple mutations to the cue list on each iteration of the outer loop.

Note that we have this same overlapping challenge in the webvtt package and we actually solve it in a slightly different way, by more lazily evaluating Subtitle#getCues (every call to that iterates over all the subtitles we have) [1]. I chatted a bit to the team, and that seems unfortunately inefficient, so I think it makes sense to keep the logic here and not copy webvtt, but maybe we can correct it and make it a bit easier to follow.

My suggestion is to get rid of insertToCueTimes() and do something more like (I haven't tested this, it might have other problems...):

(binary?) search for startTimeUs in cueTimesUs
- if startTimeUs is already there, then get the matching list (by index) from cues and add cue to it.
- else insert startTimeUs to cueTimesUs and insert a new matching list to cues (containing all the Cues from index - 1 plus cue).
Walk through cueTimesUs, adding cue to every entry matching entry in cues until you find a time that's either equal to or greater than endTimeUs (mostly your existing do/while loop)
- On each step, store a reference to the matching list of cues before you add cue. (This reference should also store the list from the else in the first bullet before cue is added)
- If the time you stopped on is equal to endTimeUs, then do nothing (the cues list already has the correct 'end' value, right?)
- If it's greater, then insert a new cues list equal to the most recent list you stored at the top of this sub-section.

[1]

ExoPlayer/library/core/src/main/java/com/google/android/exoplayer2/text/webvtt/WebvttSubtitle.java

Line 75 in 41b3fc1

public List<Cue> getCues(long timeUs) {

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java Outdated

@@ @@ -226,4 +285,15 @@ public static long parseTimecodeUs(String timeString) { @@
                   return timestampUs;
                 }
+                @Nullable
+                public static Pair<Float, Float> parsePosition(String line){

Collaborator

icbaker Oct 30, 2019

Might make more sense to use android.graphics.PointF here because it's a little less general/ambiguous than Pair<Float, Float>

Also avoids auto-boxing

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java Outdated

                 private void parseHeader(ParsableByteArray data) {
                   String currentLine;
                   while ((currentLine = data.readLine()) != null) {
+                    if (currentLine.startsWith("PlayResX:")) {
+                      playResX = Integer.valueOf(currentLine.substring(9).trim());

Collaborator

icbaker Oct 30, 2019

I think it's a tiny bit clearer to use the string literal again instead of a 'magic' length (saves a future reader manually counting the number of characters in "PlayResX:" :))

playResX = Integer.valueOf(currentLine.substring("PlayResX:".length()).trim());

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java Outdated

                     }
                   }
+                  // Parse \pos{x,y} attribute

Collaborator

icbaker Oct 30, 2019

nit: I'd move this comment to the javadoc of parsePosition

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaDecoder.java Outdated

@@ @@ -50,6 +53,9 @@ @@
                 private int formatEndIndex;
                 private int formatTextIndex;
+                private int playResX;
+                private int playResY;

Collaborator

icbaker Oct 30, 2019

It's probably safer to explicitly initialise these to an invalid value to clearly indicate 'unset', because 0 seems like a potentially genuine value we could see in a subtitle file? And then also update the comparison below on L216.

We have C.LENGTH_UNSET which I think would work well.
https://github.com/google/ExoPlayer/blob/release-v2/library/core/src/main/java/com/google/android/exoplayer2/C.java

library/core/src/main/java/com/google/android/exoplayer2/text/ssa/SsaSubtitle.java Outdated

    
                }

                @Override

                public List<Cue> getCues(long timeUs) {

                  int index = Util.binarySearchFloor(cueTimesUs, timeUs, true, false);

                  if (index == -1 || cues[index] == Cue.EMPTY) {

                  if (index == -1 || cues.get(index).isEmpty()) {

Collaborator

icbaker Oct 30, 2019

I think you can get rid of the empty check, since we'll just return the empty list below anyway (right?)

Contributor Author

szaboa Oct 30, 2019

Yes, that's right.

szaboa added 6 commits

October 30, 2019 22:45


          Use PointF instead of Pair when parsing the position

4d6d806


          Remove hardcoded index when parsing PlayResX and PlayResY

3b741e5


          Add jdoc to SSA parsePosition(..) method

86efd19


          Add initial values to playResX and playResY

7a6de79


          Remove unnecessary empty check in getCues(..)

925a7fd


          Delete accidentally pushed Project Default.xml

fb2a702

szaboa commented

View reviewed changes

Project Default.xml Outdated

Comment on lines 1 to 5

+              <component name="InspectionProjectProfileManager">
+                <profile version="1.0">
+                  <option name="myName" value="Project Default" />
+                </profile>
+              </component>

Contributor Author

szaboa Oct 30, 2019

Deleted in commit fb2a702 :)

Contributor Author

szaboa commented Oct 30, 2019

Applied the suggested changes (thank you!) except the one with same start or end time. I'll follow up on that tomorrow :)

szaboa added 2 commits

November 3, 2019 13:59


          Correct SSA overlapping subtitle decoding, add tests

0c5d470


          Merge branch 'dev-v2-ssa-position' of https://github.com/szaboa/ExoPl…

3f5654a

…ayer into dev-v2-ssa-position

Contributor Author

szaboa commented Nov 3, 2019 •

edited

Loading

I've pushed the latest changes.

Testing:
Couldn't test the code strictly on decoder level, as the parseDialogueLine is private, that's why I verified the cue times and length of cues from the decoded subtitle. Also needed to correct the "no endtime" test case, with this overlapping mechanism that use case behaves differently from now on (it carries over the previous subtitles). It is ok like this?

Overlapping challenge:

I could be wrong, but I'm not convinced this correctly handles multiple cues that have the same start or end time.

Yes, the previous implementation resulted redundant cues and cueTimes in this case, visually it was fine though.

I agree that we shouldn't add redundant things, and the algorithm you've described is more clear and bit more optimal (no need to search ahead where endTimeUs fits).

I've tried to implement it following that approach (a lot of trial and error) but couldn't really succeed because of the corner cases (e.g. handling first cue, endTimeUs is greater than all of the times, same startTime/endTime, storing reference of cues - to later add it - means we need to inspect the next+1 cueTime, not just the next one etc.) so decided to go back to the first approach and just correct the same startTime/endTime problem.

What do you think?

Collaborator

icbaker commented Nov 5, 2019

Looks good, thanks! I'll work on getting this merged.

Contributor Author

szaboa commented Nov 5, 2019

Great, let me know if any further changes are needed.

Collaborator

icbaker commented Nov 11, 2019

Just to keep you posted, I haven't forgotten about this :)

It turns out it's a little tricky to merge this while also supporting the 'blank' end timecode behaviour currently in SsaDecoder (where the intention is that the line appears only until the next line...). I've chatted with the team, and we're likely to remove the blank end timecode 'feature' as it's not really supported by the spec afaict.

Once that support is removed, I'll be able to merge this more easily.

Contributor Author

szaboa commented Nov 11, 2019

Sure, there's no hurry :)

ojw28 added the should merge label

ojw28 pushed a commit that referenced this pull request


          Require an end timecode in SSA and Subrip subtitles

ddb70d9

SSA spec allows the lines in any order, so they must all have an end time:
http://moodub.free.fr/video/ass-specs.doc

The Matroska write-up of SubRip assumes the end time is present:
https://matroska.org/technical/specs/subtitles/srt.html

This will massively simplify merging issue:#6595

PiperOrigin-RevId: 279926730

MurtadhaS commented Dec 5, 2019

Hi guys, any news regarding this PR?

icbaker added a commit that referenced this pull request


          Merge pull request #6595 from szaboa:dev-v2-ssa-position

8494c3a

PiperOrigin-RevId: 283722376

icbaker merged commit 3f5654a into google:dev-v2

Collaborator

icbaker commented Dec 5, 2019

Just merged it in to dev-v2 (with fairly significant changes, but the functionality originally proposed should all be there).

ojw28 pushed a commit that referenced this pull request


          Merge pull request #6595 from szaboa:dev-v2-ssa-position

6a354bb

PiperOrigin-RevId: 283722376

google locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes should merge