#4306 - Extract tags from SubRip subtitles, add support for alignment #4582

szaboa · 2018-07-26T16:48:56Z

Extracted valid tags from srt subtitles (so those won't be rendered anymore) and added support for different alignments tags (based on SSA v4+ specification) such as {\an2} for example.

…gnment tags based on SSA v4+

ojw28 · 2018-08-06T15:53:46Z

library/core/src/test/java/com/google/android/exoplayer2/text/subrip/SubripDecoderTest.java

@@ -154,6 +155,27 @@ public void testDecodeNoEndTimecodes() throws IOException {
        .isEqualTo("Or to the end of the media.");
  }

+  @Test
+  public void testDecodeCueWithTag() throws IOException{


This test doesn't appear to be asserting anything other than that the tags were removed (or not) from the text. Shouldn't the test also be asserting that they were applied successfully (e.g. that the subtitles in the cues have the right alignment parameters)

ojw28 · 2018-08-06T15:55:58Z

library/core/src/test/java/com/google/android/exoplayer2/text/subrip/SubripDecoderTest.java

+    assertThat(subtitle.getCues(subtitle.getEventTime(4)).get(0).text.toString())
+        .isEqualTo("This is the third subtitle.");
+
+    // Based on the SSA v4+ specs the curly bracket must be followed by a backslash, so this is


Can you link to this doc (just in this thread, not in the source). The doc I found has a bunch of examples where this is always true, but it's also always true that there's no whitespace inside the brackets either. Hence it seems strange to forbid { \\an2} but not {\ an2}. Unless you have counter examples, I think we should just require no whitespace in the bracketed region. Which I think you can do by using [^\s] instead of . in the Pattern.

I followed the docs that can be downloaded from here:
moodub.free.fr/video/ass-specs.doc

And it says: "All override codes are always preceded by a backslash "
So that's why I didn't allow a space after the bracket. Regarding spaces after the backslash I just followed how the VLC handles these cases.

We sure can require no whitespace in the bracketed region, is up to you :)

Guess we can allow space, if there's an existing example of doing this (as there seems to be)!

ojw28 · 2018-08-06T16:08:48Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

-      Spanned text = Html.fromHtml(textBuilder.toString());
-      cues.add(new Cue(text));
+      // Extract tags
+      SubtitleTagResult tagResult = extractTags(textBuilder);


Given you wouldn't expect a tag to be separated over multiple lines, it seems cleaner to do this in the while loop above. You could replace the last line in the while loop with something like:

textBuilder.append(processLine(currentLine, tags));

Where tags is an Arraylist to which any new tags are added, and processLine is responsible for trimming the line and extracting tags. Note passing the list in as an argument also avoids having to define SubtitleTagResult.

You'd then make the tags list before the start of the while loop to pass in (you could even re-use the same single instance if you want, as is done for the StringBuilder, as long as you remember to clear it.

ojw28 · 2018-08-06T16:09:33Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+      Cue cue = null;
+
+      // Check if tags are present
+      if (tagResult.tags.length > 0) {


nit - You can probably remove this. The loop will be a no-op if tags has length zero.

ojw28 · 2018-08-06T16:10:17Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+      // Check if tags are present
+      if (tagResult.tags.length > 0) {
+
+        boolean alignTagFound = false;


Can't you just break; from the loop when you find the first align tag, and avoid the need for this?

Sure I could, but I intended to wrote like this to be generic and to be able to extend the loop with other tag's "apply" logic. Note that this for loop isn't only for align tags (for now it is), instead goes over all the tags and applies them, I suppose it will have support for more tags in the future and it that case breaking after the first align tag would be incorrect behavior.

Taking these into consideration, if you still would like to just "break", then I can change it.

I think our preference (for small blocks of code; this doesn't really apply to larger scale design choices for obvious reasons) is to write it in the simplest way for current usage, and change it as and when the usage changes. So I think using break is still preferable for now.

ojw28 · 2018-08-06T16:12:42Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+            if (alignTagFound) continue;
+            alignTagFound = true;
+
+            AlignmentResult alignmentResult = getAlignmentValues(tag);


If you change getAlignmentValues(tag) to be buildCue(Spanned text, String alignmentTag) then you can get rid of AlignmentResult.

szaboa · 2018-08-30T19:58:40Z

@ojw28 waiting for your opinion on the break; thing, meanwhile I will extend the test cases. The rest is resolved.

ojw28 · 2018-09-03T08:08:04Z

Replied to the break; thing. Please push the updated revision when ready. Thanks!

ojw28 · 2018-09-23T17:53:13Z

@szaboa - I think there's still an open comment on testDecodeCueWithTag. Perhaps you're aware of this already; just checking!

szaboa · 2018-09-24T11:09:45Z

Yes, there is that and the break thing. I am on vacation right now, I will resolve those on the next Monday (first of October) when I am back, sorry for the delays!

…ations

szaboa · 2018-10-01T20:20:25Z

@ojw28
Just pushed the changes for the remaining open comments. Everything should be OK now.

ojw28

Looks good, thanks. I put a few final comments inline.

ojw28 · 2018-10-01T20:34:12Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+        break;
+    }
+
+    return new Cue(text, Layout.Alignment.ALIGN_NORMAL, line, Cue.LINE_TYPE_FRACTION, lineAnchor, position, positionAnchor, Cue.DIMEN_UNSET);


Can you use null for text alignment? Cue implies that's what you should use unless you know better.

ojw28 · 2018-10-01T20:43:04Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+        line = DEFAULT_END_FRACTION;
+        position = DEFAULT_START_FRACTION;
+        positionAnchor = Cue.ANCHOR_TYPE_START;
+        lineAnchor = Cue.ANCHOR_TYPE_END;


Would be nice to order the assignments as line,lineAnchor,position,positionAnchor so the line and position ones are grouped. Ignorable, but it might be even clearer if split into two switch blocks with three cases each, like:

float position; @Cue.AnchorType int positionAnchor; switch (alignmentTag) { case ALIGN_BOTTOM_LEFT: case ALIGN_MID_LEFT: case ALIGN_TOP_LEFT: position = DEFAULT_START_FRACTION; positionAnchor = Cue.ANCHOR_TYPE_START; case ALIGN_BOTTOM_MID: case ALIGN_MID_MID: case ALIGN_TOP_MID: ... case ALIGN_BOTTOM_RIGHT: case ALIGN_MID_RIGHT: case ALIGN_TOP_RIGHT: ... } ... (ditto for line and lineAnchor) ...

Yep, much nicer!

ojw28 · 2018-10-01T20:46:02Z

library/core/src/main/java/com/google/android/exoplayer2/text/subrip/SubripDecoder.java

+   */
+  private Cue buildCue(Spanned text, String alignmentTag) {
+    // Default values used for positioning the subtitle in case of align tags
+    float line = DEFAULT_END_FRACTION, position = DEFAULT_MID_FRACTION;


No need to assign initial values to any of these (it's better not to, since that'll make the compiler ensure they're initialized along all code paths below). Add default: to the last case in the switch statement if needed to make the compiler happy.

…signments, setting Cue's textAlignment to def value - null

szaboa · 2018-10-02T15:47:48Z

Pushed the changes according to the comments.

ojw28 · 2018-10-03T21:20:17Z

Thanks! We made some further improvements during internal review, which were merged in 7849a5e. Let us know if we accidentally broken anything :).

szaboa · 2018-10-04T16:35:51Z

It works fine and looks much better now, actually I am feeling that I did poor job after seeing your changes :)

Anyway, based on this I will try to pick up a better coding style in my next pull request.
Cheers!

ojw28 · 2018-10-04T16:47:50Z

Not at all; this feature wouldn't have happened without you ;). Thanks for the contribution!

#4306 - Extract tags from SubRip subtitles, add support for alignment

google#4306 - Extract tags from SubRip subtitles, add support for ali…

ae520a8

…gnment tags based on SSA v4+

googlebot added the cla: yes label Jul 26, 2018

szaboa mentioned this pull request Jul 26, 2018

SRT subs position tags #4306

Closed

ojw28 reviewed Aug 6, 2018

View reviewed changes

ojw28 added the pending comments label Aug 6, 2018

tonihei assigned ojw28 Sep 3, 2018

google#4306 - code review fixes

876080e

szaboa added 2 commits October 1, 2018 22:45

google#4306 - breaking after the first alignment tag is found

fc5eb12

google#4306 - extends test case with line and position anchor verific…

75a7385

…ations

ojw28 reviewed Oct 1, 2018

View reviewed changes

google#4306 - grouping line/lineAnchor and position/positionAnchor as…

56c7e1f

…signments, setting Cue's textAlignment to def value - null

ojw28 added should merge and removed pending comments labels Oct 2, 2018

ojw28 merged commit 16fe67b into google:dev-v2 Oct 3, 2018

ojw28 added a commit that referenced this pull request Oct 20, 2018

Merge pull request #4582 from szaboa/feature/4306_srt_position_tags

4d6b008

#4306 - Extract tags from SubRip subtitles, add support for alignment

google locked and limited conversation to collaborators May 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#4306 - Extract tags from SubRip subtitles, add support for alignment #4582

#4306 - Extract tags from SubRip subtitles, add support for alignment #4582

szaboa commented Jul 26, 2018

ojw28 Aug 6, 2018

ojw28 Aug 6, 2018

szaboa Aug 7, 2018

ojw28 Aug 7, 2018

ojw28 Aug 6, 2018

ojw28 Aug 6, 2018

ojw28 Aug 6, 2018

szaboa Aug 30, 2018 •

edited

Loading

ojw28 Sep 3, 2018

ojw28 Aug 6, 2018

szaboa commented Aug 30, 2018

ojw28 commented Sep 3, 2018 •

edited

Loading

ojw28 commented Sep 23, 2018

szaboa commented Sep 24, 2018

szaboa commented Oct 1, 2018

ojw28 left a comment

ojw28 Oct 1, 2018

ojw28 Oct 1, 2018

szaboa Oct 2, 2018

ojw28 Oct 1, 2018

szaboa commented Oct 2, 2018

ojw28 commented Oct 3, 2018

szaboa commented Oct 4, 2018

ojw28 commented Oct 4, 2018

#4306 - Extract tags from SubRip subtitles, add support for alignment #4582

#4306 - Extract tags from SubRip subtitles, add support for alignment #4582

Conversation

szaboa commented Jul 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szaboa Aug 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szaboa commented Aug 30, 2018

ojw28 commented Sep 3, 2018 • edited Loading

ojw28 commented Sep 23, 2018

szaboa commented Sep 24, 2018

szaboa commented Oct 1, 2018

ojw28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szaboa commented Oct 2, 2018

ojw28 commented Oct 3, 2018

szaboa commented Oct 4, 2018

ojw28 commented Oct 4, 2018

szaboa Aug 30, 2018 •

edited

Loading

ojw28 commented Sep 3, 2018 •

edited

Loading