From 27576cfc73814865cab7ff70baaae9a1f3461a97 Mon Sep 17 00:00:00 2001
From: Denise LaFayette <dlafayette@netflix.com>
Date: Mon, 19 Jul 2021 12:18:03 -0400
Subject: [PATCH] Fix bug where rubyPosition in text node is not applied

Also fix bug where rubyPosition in ruby container style is not applied
---
 .../exoplayer2/text/ttml/TtmlRenderUtil.java  | 17 +++++++++----
 .../exoplayer2/text/ttml/TtmlDecoderTest.java | 24 ++++++++++++++-----
 .../src/test/assets/media/ttml/rubies.xml     | 24 +++++++++++++++++++
 3 files changed, 55 insertions(+), 10 deletions(-)

diff --git a/library/core/src/main/java/com/google/android/exoplayer2/text/ttml/TtmlRenderUtil.java b/library/core/src/main/java/com/google/android/exoplayer2/text/ttml/TtmlRenderUtil.java
index b7f4b0f5b1..86978616e4 100644
--- a/library/core/src/main/java/com/google/android/exoplayer2/text/ttml/TtmlRenderUtil.java
+++ b/library/core/src/main/java/com/google/android/exoplayer2/text/ttml/TtmlRenderUtil.java
@@ -178,12 +178,21 @@ import java.util.Map;
           break;
         }
 
-        // TODO: Get rubyPosition from `textNode` when TTML inheritance is implemented.
+        @Nullable TtmlStyle textStyle = resolveStyle(textNode.style, textNode.getStyleIds(),
+            globalStyles);
+
+        // Use position from ruby text node if defined
         @TextAnnotation.Position
         int rubyPosition =
-            containerNode.style != null
-                ? containerNode.style.getRubyPosition()
-                : TextAnnotation.POSITION_UNKNOWN;
+            textStyle != null ? textStyle.getRubyPosition() : TextAnnotation.POSITION_UNKNOWN;
+
+        if (rubyPosition == TextAnnotation.POSITION_UNKNOWN) {
+          // If ruby position is not defined, use position info from container node
+          @Nullable TtmlStyle containerStyle = resolveStyle(containerNode.style,
+              containerNode.getStyleIds(), globalStyles);
+          rubyPosition = containerStyle != null ? containerStyle.getRubyPosition() : rubyPosition;
+        }
+
         builder.setSpan(
             new RubySpan(rubyText, rubyPosition), start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE);
         break;
diff --git a/library/core/src/test/java/com/google/android/exoplayer2/text/ttml/TtmlDecoderTest.java b/library/core/src/test/java/com/google/android/exoplayer2/text/ttml/TtmlDecoderTest.java
index d068ade74b..5d2d936872 100644
--- a/library/core/src/test/java/com/google/android/exoplayer2/text/ttml/TtmlDecoderTest.java
+++ b/library/core/src/test/java/com/google/android/exoplayer2/text/ttml/TtmlDecoderTest.java
@@ -37,7 +37,9 @@ import java.util.Map;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-/** Unit test for {@link TtmlDecoder}. */
+/**
+ * Unit test for {@link TtmlDecoder}.
+ */
 @RunWith(AndroidJUnit4.class)
 public final class TtmlDecoderTest {
 
@@ -114,11 +116,9 @@ public final class TtmlDecoderTest {
    * </code>.
    *
    * @throws IOException thrown if reading subtitle file fails.
-   * @see <a
-   *     href="https://github.com/android/platform_frameworks_base/blob/jb-mr2-release/graphics/java/android/graphics/Color.java#L414">
-   *     JellyBean Color</a> <a
-   *     href="https://github.com/android/platform_frameworks_base/blob/kitkat-mr2.2-release/graphics/java/android/graphics/Color.java#L414">
-   *     Kitkat Color</a>
+   * @see <a href="https://github.com/android/platform_frameworks_base/blob/jb-mr2-release/graphics/java/android/graphics/Color.java#L414">
+   * JellyBean Color</a> <a href="https://github.com/android/platform_frameworks_base/blob/kitkat-mr2.2-release/graphics/java/android/graphics/Color.java#L414">
+   * Kitkat Color</a>
    */
   @Test
   public void lime() throws IOException, SubtitleDecoderException {
@@ -710,6 +710,18 @@ public final class TtmlDecoderTest {
     Spanned sixthCue = getOnlyCueTextAtTimeUs(subtitle, 60_000_000);
     assertThat(sixthCue.toString()).isEqualTo("Cue with annotated text.");
     assertThat(sixthCue).hasNoRubySpanBetween(0, sixthCue.length());
+
+    Spanned seventhCue = getOnlyCueTextAtTimeUs(subtitle, 70_000_000);
+    assertThat(seventhCue.toString()).isEqualTo("Cue with annotated text.");
+    assertThat(seventhCue)
+        .hasRubySpanBetween("Cue with ".length(), "Cue with annotated".length())
+        .withTextAndPosition("rubies", TextAnnotation.POSITION_BEFORE);
+
+    Spanned eighthCue = getOnlyCueTextAtTimeUs(subtitle, 80_000_000);
+    assertThat(eighthCue.toString()).isEqualTo("Cue with annotated text.");
+    assertThat(eighthCue)
+        .hasRubySpanBetween("Cue with ".length(), "Cue with annotated".length())
+        .withTextAndPosition("rubies", TextAnnotation.POSITION_AFTER);
   }
 
   @Test
diff --git a/testdata/src/test/assets/media/ttml/rubies.xml b/testdata/src/test/assets/media/ttml/rubies.xml
index e149f16dfd..71ba022a65 100644
--- a/testdata/src/test/assets/media/ttml/rubies.xml
+++ b/testdata/src/test/assets/media/ttml/rubies.xml
@@ -7,6 +7,8 @@
       <style id="cont" tts:ruby="container" />
       <style id="base" tts:ruby="base" />
       <style id="text" tts:ruby="text" />
+      <style id="contpos" tts:ruby="container" tts:rubyPosition="before" />
+      <style id="textpos" tts:ruby="text" tts:rubyPosition="after" />
     </styling>
   </head>
   <body>
@@ -76,5 +78,27 @@
         text.
       </p>
     </div>
+    <div>
+      <!-- ruby info in style block and ruby position in contpos -->
+      <p begin="70s" end="78s">
+        Cue with
+        <span style="contpos">
+          <span style="base">annotated</span>
+          <span style="text">rubies</span>
+          text.
+        </span>
+      </p>
+    </div>
+    <div>
+      <!-- ruby info and position in style block for base and text -->
+      <p begin="80s" end="88s">
+        Cue with
+        <span style="contpos">
+          <span style="base">annotated</span>
+          <span style="textpos">rubies</span>
+          text.
+        </span>
+      </p>
+    </div>
   </body>
 </tt>