From bb2fd002aef583b10eba9c497770c87a3be38839 Mon Sep 17 00:00:00 2001 From: ibaker Date: Fri, 5 Jul 2024 05:54:46 -0700 Subject: [PATCH] Fix TTML handling of inherited percentage `tts:fontSize` values The percentage should be interpreted as relative to the size of a parent node. This change makes this inheritance work correctly for percentages in both the parent and child. It does not fix the case of a non-percentage parent size with a percentage child size. PiperOrigin-RevId: 649631055 --- RELEASENOTES.md | 3 + .../androidx/media3/common/text/SpanUtil.java | 49 ++++++++++-- .../media3/common/text/SpanUtilTest.java | 74 +++++++++++++++++++ ...legatingSubtitleDecoderTtmlParserTest.java | 19 ++++- .../extractor/text/ttml/TtmlRenderUtil.java | 8 +- .../extractor/text/ttml/TtmlParserTest.java | 18 ++++- .../media/ttml/inherit_and_override_style.xml | 49 ++++++------ .../test/utils/truth/SpannedSubject.java | 6 +- 8 files changed, 187 insertions(+), 39 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index e1d70f9745..e7ea5063d9 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -14,6 +14,9 @@ * Audio: * Video: * Text: + * TTML: Fix handling of percentage `tts:fontSize` values to ensure they + are correctly inherited from parent nodes with percentage `tts:fontSize` + values. * Metadata: * Image: * DataSource: diff --git a/libraries/common/src/main/java/androidx/media3/common/text/SpanUtil.java b/libraries/common/src/main/java/androidx/media3/common/text/SpanUtil.java index dd73c5904b..cbf2d1cc58 100644 --- a/libraries/common/src/main/java/androidx/media3/common/text/SpanUtil.java +++ b/libraries/common/src/main/java/androidx/media3/common/text/SpanUtil.java @@ -17,6 +17,7 @@ package androidx.media3.common.text; import android.text.Spannable; import android.text.style.ForegroundColorSpan; +import android.text.style.RelativeSizeSpan; import androidx.media3.common.util.UnstableApi; /** @@ -44,14 +45,52 @@ public final class SpanUtil { Spannable spannable, Object span, int start, int end, int spanFlags) { Object[] existingSpans = spannable.getSpans(start, end, span.getClass()); for (Object existingSpan : existingSpans) { - if (spannable.getSpanStart(existingSpan) == start - && spannable.getSpanEnd(existingSpan) == end - && spannable.getSpanFlags(existingSpan) == spanFlags) { - spannable.removeSpan(existingSpan); - } + removeIfStartEndAndFlagsMatch(spannable, existingSpan, start, end, spanFlags); } spannable.setSpan(span, start, end, spanFlags); } + /** + * Modifies the size of the text between {@code start} and {@code end} relative to any existing + * {@link RelativeSizeSpan} instances which cover at least the same range. + * + *

{@link RelativeSizeSpan} instances which only cover a part of the text between {@code start} + * and {@code end} are ignored. + * + *

A new {@link RelativeSizeSpan} instance is added between {@code start} and {@code end} with + * its {@code sizeChange} value computed by modifying the {@code size} parameter by the {@code + * sizeChange} of {@link RelativeSizeSpan} instances covering between {@code start} and {@code + * end}. + * + *

{@link RelativeSizeSpan} instances with the same {@code start}, {@code end}, and {@code + * spanFlags} are removed. + * + * @param spannable The {@link Spannable} to add the {@link RelativeSizeSpan} to. + * @param size The fraction to modify the text size by. + * @param start The start index to add the new span at. + * @param end The end index to add the new span at. + * @param spanFlags The flags to pass to {@link Spannable#setSpan(Object, int, int, int)}. + */ + public static void addInheritedRelativeSizeSpan( + Spannable spannable, float size, int start, int end, int spanFlags) { + for (RelativeSizeSpan existingSpan : spannable.getSpans(start, end, RelativeSizeSpan.class)) { + if (spannable.getSpanStart(existingSpan) <= start + && spannable.getSpanEnd(existingSpan) >= end) { + size *= existingSpan.getSizeChange(); + } + removeIfStartEndAndFlagsMatch(spannable, existingSpan, start, end, spanFlags); + } + spannable.setSpan(new RelativeSizeSpan(size), start, end, spanFlags); + } + + private static void removeIfStartEndAndFlagsMatch( + Spannable spannable, Object span, int start, int end, int spanFlags) { + if (spannable.getSpanStart(span) == start + && spannable.getSpanEnd(span) == end + && spannable.getSpanFlags(span) == spanFlags) { + spannable.removeSpan(span); + } + } + private SpanUtil() {} } diff --git a/libraries/common/src/test/java/androidx/media3/common/text/SpanUtilTest.java b/libraries/common/src/test/java/androidx/media3/common/text/SpanUtilTest.java index 1197832d40..14c1153bc5 100644 --- a/libraries/common/src/test/java/androidx/media3/common/text/SpanUtilTest.java +++ b/libraries/common/src/test/java/androidx/media3/common/text/SpanUtilTest.java @@ -15,6 +15,7 @@ */ package androidx.media3.common.text; +import static androidx.media3.test.utils.truth.SpannedSubject.assertThat; import static com.google.common.truth.Truth.assertThat; import android.graphics.Color; @@ -23,6 +24,7 @@ import android.text.SpannableString; import android.text.Spanned; import android.text.style.BackgroundColorSpan; import android.text.style.ForegroundColorSpan; +import android.text.style.RelativeSizeSpan; import androidx.test.ext.junit.runners.AndroidJUnit4; import org.junit.Test; import org.junit.runner.RunWith; @@ -83,4 +85,76 @@ public class SpanUtilTest { .containsExactly(originalSpan, differentStart, differentEnd, differentFlags) .inOrder(); } + + @Test + public void addInheritedRelativeSizeSpan_noExistingSpans() { + Spannable spannable = SpannableString.valueOf("test text"); + + SpanUtil.addInheritedRelativeSizeSpan( + spannable, + /* size= */ 0.5f, + /* start= */ 2, + /* end= */ 5, + Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + assertThat(spannable).hasRelativeSizeSpanBetween(2, 5).withSizeChange(0.5f); + } + + @Test + public void addInheritedRelativeSizeSpan_existingSpanWithSameRange_replaced() { + Spannable spannable = SpannableString.valueOf("test text"); + spannable.setSpan( + new RelativeSizeSpan(1.6f), /* start= */ 2, /* end= */ 5, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + SpanUtil.addInheritedRelativeSizeSpan( + spannable, + /* size= */ 0.5f, + /* start= */ 2, + /* end= */ 5, + Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + RelativeSizeSpan[] spans = spannable.getSpans(2, 5, RelativeSizeSpan.class); + assertThat(spans).hasLength(1); + assertThat(spans[0].getSizeChange()).isWithin(0.0000001f).of(0.8f); + } + + @Test + public void addInheritedRelativeSizeSpan_existingLongerSpan() { + Spannable spannable = SpannableString.valueOf("test text"); + spannable.setSpan( + new RelativeSizeSpan(1.6f), /* start= */ 1, /* end= */ 6, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + SpanUtil.addInheritedRelativeSizeSpan( + spannable, + /* size= */ 0.5f, + /* start= */ 2, + /* end= */ 5, + Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + RelativeSizeSpan[] spans = spannable.getSpans(2, 5, RelativeSizeSpan.class); + assertThat(spans).hasLength(2); + assertThat(spannable).hasRelativeSizeSpanBetween(2, 5).withSizeChange(0.8f); + } + + @Test + public void addInheritedRelativeSizeSpan_existingIncompleteSpans_ignored() { + Spannable spannable = SpannableString.valueOf("test text"); + spannable.setSpan( + new RelativeSizeSpan(2.3f), /* start= */ 1, /* end= */ 4, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + spannable.setSpan( + new RelativeSizeSpan(1.6f), /* start= */ 3, /* end= */ 4, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + spannable.setSpan( + new RelativeSizeSpan(2.3f), /* start= */ 3, /* end= */ 6, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + SpanUtil.addInheritedRelativeSizeSpan( + spannable, + /* size= */ 0.5f, + /* start= */ 2, + /* end= */ 5, + Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + + RelativeSizeSpan[] spans = spannable.getSpans(2, 5, RelativeSizeSpan.class); + assertThat(spans).hasLength(4); + assertThat(spannable).hasRelativeSizeSpanBetween(2, 5).withSizeChange(0.5f); + } } diff --git a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/text/DelegatingSubtitleDecoderTtmlParserTest.java b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/text/DelegatingSubtitleDecoderTtmlParserTest.java index 6e8a87c745..cc2a942d83 100644 --- a/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/text/DelegatingSubtitleDecoderTtmlParserTest.java +++ b/libraries/exoplayer/src/test/java/androidx/media3/exoplayer/text/DelegatingSubtitleDecoderTtmlParserTest.java @@ -28,6 +28,7 @@ import androidx.media3.common.util.ColorParser; import androidx.media3.extractor.text.Subtitle; import androidx.media3.extractor.text.ttml.TtmlParser; import androidx.media3.test.utils.TestUtil; +import androidx.media3.test.utils.truth.SpannedSubject; import androidx.test.core.app.ApplicationProvider; import androidx.test.ext.junit.runners.AndroidJUnit4; import java.io.IOException; @@ -149,10 +150,10 @@ public final class DelegatingSubtitleDecoderTtmlParserTest { public void inheritGlobalStyleOverriddenByInlineAttributes() throws IOException { Subtitle subtitle = getSubtitle(INHERIT_STYLE_OVERRIDE_TTML_FILE); - assertThat(subtitle.getEventTimeCount()).isEqualTo(4); + assertThat(subtitle.getEventTimeCount()).isEqualTo(6); Spanned firstCueText = getOnlyCueTextAtTimeUs(subtitle, 10_000_000); - assertThat(firstCueText.toString()).isEqualTo("text 1"); + assertThat(firstCueText.toString()).isEqualTo("default + s0 styles"); assertThat(firstCueText).hasTypefaceSpanBetween(0, firstCueText.length()).withFamily("serif"); assertThat(firstCueText).hasBoldItalicSpanBetween(0, firstCueText.length()); assertThat(firstCueText).hasUnderlineSpanBetween(0, firstCueText.length()); @@ -162,9 +163,12 @@ public final class DelegatingSubtitleDecoderTtmlParserTest { assertThat(firstCueText) .hasForegroundColorSpanBetween(0, firstCueText.length()) .withColor(0xFFFFFF00); + SpannedSubject.assertThat(firstCueText) + .hasRelativeSizeSpanBetween(0, firstCueText.length()) + .withSizeChange(1.5f); Spanned secondCueText = getOnlyCueTextAtTimeUs(subtitle, 20_000_000); - assertThat(secondCueText.toString()).isEqualTo("text 2"); + assertThat(secondCueText.toString()).isEqualTo("default + s0 + overrides"); assertThat(secondCueText) .hasTypefaceSpanBetween(0, secondCueText.length()) .withFamily("sansSerif"); @@ -176,6 +180,15 @@ public final class DelegatingSubtitleDecoderTtmlParserTest { assertThat(secondCueText) .hasForegroundColorSpanBetween(0, secondCueText.length()) .withColor(0xFFFFFF00); + SpannedSubject.assertThat(secondCueText) + .hasRelativeSizeSpanBetween(0, secondCueText.length()) + .withSizeChange(0.9f); + + Spanned thirdCueText = getOnlyCueTextAtTimeUs(subtitle, 30_000_000); + assertThat(thirdCueText.toString()).isEqualTo("default styling only"); + assertThat(thirdCueText) + .hasRelativeSizeSpanBetween(0, thirdCueText.length()) + .withSizeChange(0.75f); } @Test diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/ttml/TtmlRenderUtil.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/ttml/TtmlRenderUtil.java index d7e02d9b8c..2931056bfe 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/ttml/TtmlRenderUtil.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/ttml/TtmlRenderUtil.java @@ -239,12 +239,8 @@ import java.util.Map; Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); break; case TtmlStyle.FONT_SIZE_UNIT_PERCENT: - SpanUtil.addOrReplaceSpan( - builder, - new RelativeSizeSpan(style.getFontSize() / 100), - start, - end, - Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); + SpanUtil.addInheritedRelativeSizeSpan( + builder, style.getFontSize() / 100, start, end, Spanned.SPAN_EXCLUSIVE_EXCLUSIVE); break; case TtmlStyle.UNSPECIFIED: // Do nothing. diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/ttml/TtmlParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/ttml/TtmlParserTest.java index d3b24b531c..08945d44d7 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/ttml/TtmlParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/ttml/TtmlParserTest.java @@ -325,10 +325,10 @@ public final class TtmlParserTest { public void inheritGlobalStyleOverriddenByInlineAttributes() throws Exception { ImmutableList allCues = getAllCues(INHERIT_STYLE_OVERRIDE_TTML_FILE); - assertThat(allCues).hasSize(2); + assertThat(allCues).hasSize(3); Spanned firstCueText = getOnlyCueTextAtIndex(allCues, 0); - assertThat(firstCueText.toString()).isEqualTo("text 1"); + assertThat(firstCueText.toString()).isEqualTo("default + s0 styles"); assertThat(firstCueText).hasTypefaceSpanBetween(0, firstCueText.length()).withFamily("serif"); assertThat(firstCueText).hasBoldItalicSpanBetween(0, firstCueText.length()); assertThat(firstCueText).hasUnderlineSpanBetween(0, firstCueText.length()); @@ -338,9 +338,12 @@ public final class TtmlParserTest { assertThat(firstCueText) .hasForegroundColorSpanBetween(0, firstCueText.length()) .withColor(0xFFFFFF00); + assertThat(firstCueText) + .hasRelativeSizeSpanBetween(0, firstCueText.length()) + .withSizeChange(1.5f); Spanned secondCueText = getOnlyCueTextAtIndex(allCues, 1); - assertThat(secondCueText.toString()).isEqualTo("text 2"); + assertThat(secondCueText.toString()).isEqualTo("default + s0 + overrides"); assertThat(secondCueText) .hasTypefaceSpanBetween(0, secondCueText.length()) .withFamily("sansSerif"); @@ -352,6 +355,15 @@ public final class TtmlParserTest { assertThat(secondCueText) .hasForegroundColorSpanBetween(0, secondCueText.length()) .withColor(0xFFFFFF00); + assertThat(secondCueText) + .hasRelativeSizeSpanBetween(0, secondCueText.length()) + .withSizeChange(0.9f); + + Spanned thirdCueText = getOnlyCueTextAtIndex(allCues, 2); + assertThat(thirdCueText.toString()).isEqualTo("default styling only"); + assertThat(thirdCueText) + .hasRelativeSizeSpanBetween(0, thirdCueText.length()) + .withSizeChange(0.75f); } @Test diff --git a/libraries/test_data/src/test/assets/media/ttml/inherit_and_override_style.xml b/libraries/test_data/src/test/assets/media/ttml/inherit_and_override_style.xml index 838c8b9822..adb1eceddc 100644 --- a/libraries/test_data/src/test/assets/media/ttml/inherit_and_override_style.xml +++ b/libraries/test_data/src/test/assets/media/ttml/inherit_and_override_style.xml @@ -2,28 +2,35 @@ xmlns:ttp="http://www.w3.org/2006/10/ttaf1#parameter" xmlns:tts="http://www.w3.org/2006/10/ttaf1#style" xmlns:ttm="http://www.w3.org/2006/10/ttaf1#metadata"> - - -