From 97d3d50445b382cb2f017b87e666f08321abc104 Mon Sep 17 00:00:00 2001 From: hoyahozz <85336456+hoyahozz@users.noreply.github.com> Date: Sun, 9 Aug 2026 22:31:48 +0900 Subject: [PATCH 1/7] Support custom charset detection for SubRip subtitles Standalone SubRip subtitles without a byte order mark are decoded as UTF-8, which corrupts files that use legacy character encodings. Add an injectable CharsetDetector to DefaultSubtitleParserFactory and use it when parsing standalone SubRip files. Keep BOM precedence, UTF-8 fallback, and embedded Matroska/WebM subtitle behavior unchanged. Issue: androidx/media#2247 --- RELEASENOTES.md | 4 + .../extractor/text/CharsetDetector.java | 38 ++++++++ .../text/DefaultSubtitleParserFactory.java | 29 +++++- .../extractor/text/subrip/SubripParser.java | 41 ++++++-- .../DefaultSubtitleParserFactoryTest.java | 67 +++++++++++++ .../text/subrip/SubripParserTest.java | 96 +++++++++++++++++++ 6 files changed, 268 insertions(+), 7 deletions(-) create mode 100644 libraries/extractor/src/main/java/androidx/media3/extractor/text/CharsetDetector.java diff --git a/RELEASENOTES.md b/RELEASENOTES.md index da99134082e..5d11f7770f2 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -112,6 +112,10 @@ be changed during playback with `Player.replaceMediaItem(int, MediaItem)` without interrupting playback ([#1976](https://github.com/androidx/media/issues/1976)). + * SubRip: Add support for injecting a `CharsetDetector` into + `DefaultSubtitleParserFactory` to detect the character encoding of + standalone SubRip subtitles without a byte order mark + ([#2247](https://github.com/androidx/media/issues/2247)). * Metadata: * Image: * DataSource: diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/CharsetDetector.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/CharsetDetector.java new file mode 100644 index 00000000000..0d6fb82d701 --- /dev/null +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/CharsetDetector.java @@ -0,0 +1,38 @@ +/* + * Copyright 2026 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package androidx.media3.extractor.text; + +import androidx.annotation.Nullable; +import androidx.media3.common.util.UnstableApi; +import java.nio.charset.Charset; + +/** Detects the character encoding of subtitle byte data. */ +@UnstableApi +public interface CharsetDetector { + + /** + * Detects the character encoding of the requested range of {@code data}. + * + *

The requested range is not guaranteed to contain a complete subtitle file. + * + * @param data The subtitle byte data. + * @param offset The start offset in {@code data}. + * @param length The number of bytes to inspect. + * @return The detected character encoding, or {@code null} if it could not be determined. + */ + @Nullable + Charset detect(byte[] data, int offset, int length); +} diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java index b2413e3be55..9eaa75eedde 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java @@ -46,10 +46,33 @@ *

  • DVB ({@link DvbParser}) *
  • TTML ({@link TtmlParser}) * + * + *

    A {@link CharsetDetector} can be provided to detect the character encoding of standalone + * SubRip subtitles without a byte order mark. */ @UnstableApi public final class DefaultSubtitleParserFactory implements SubtitleParser.Factory { + @Nullable private final CharsetDetector charsetDetector; + + /** Creates an instance that defaults to UTF-8 for SubRip subtitles without a byte order mark. */ + public DefaultSubtitleParserFactory() { + this(/* charsetDetector= */ null); + } + + /** + * Creates an instance that uses {@code charsetDetector} for standalone SubRip subtitles without a + * byte order mark. + * + *

    The detector is not used for SubRip subtitles embedded in a media container because these + * samples may contain only a small part of the subtitle file. + * + * @param charsetDetector The detector to use, or {@code null} to default to UTF-8. + */ + public DefaultSubtitleParserFactory(@Nullable CharsetDetector charsetDetector) { + this.charsetDetector = charsetDetector; + } + @Override public boolean supportsFormat(Format format) { @Nullable String mimeType = format.sampleMimeType; @@ -106,7 +129,7 @@ public SubtitleParser create(Format format) { case MimeTypes.APPLICATION_MP4VTT: return new Mp4WebvttParser(); case MimeTypes.APPLICATION_SUBRIP: - return new SubripParser(); + return new SubripParser(isStandaloneSubrip(format) ? charsetDetector : null); case MimeTypes.APPLICATION_TX3G: return new Tx3gParser(format.initializationData); case MimeTypes.APPLICATION_PGS: @@ -123,4 +146,8 @@ public SubtitleParser create(Format format) { } throw new IllegalArgumentException("Unsupported MIME type: " + mimeType); } + + private static boolean isStandaloneSubrip(Format format) { + return format.containerMimeType == null || MimeTypes.isText(format.containerMimeType); + } } diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java index 8f607ed6357..16415c73c0f 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java @@ -31,6 +31,7 @@ import androidx.media3.common.util.Log; import androidx.media3.common.util.ParsableByteArray; import androidx.media3.common.util.UnstableApi; +import androidx.media3.extractor.text.CharsetDetector; import androidx.media3.extractor.text.CuesWithTiming; import androidx.media3.extractor.text.SubtitleParser; import com.google.common.collect.ImmutableList; @@ -82,11 +83,24 @@ public final class SubripParser implements SubtitleParser { private final StringBuilder textBuilder; private final ArrayList tags; private final ParsableByteArray parsableByteArray; + @Nullable private final CharsetDetector charsetDetector; public SubripParser() { + this(/* charsetDetector= */ null); + } + + /** + * Creates an instance that uses {@code charsetDetector} when the input doesn't contain a byte + * order mark. + * + * @param charsetDetector The detector to use, or {@code null} to default to UTF-8 when the input + * doesn't contain a byte order mark. + */ + public SubripParser(@Nullable CharsetDetector charsetDetector) { textBuilder = new StringBuilder(); tags = new ArrayList<>(); parsableByteArray = new ParsableByteArray(); + this.charsetDetector = charsetDetector; } @Override @@ -103,7 +117,7 @@ public void parse( Consumer output) { parsableByteArray.reset(data, /* limit= */ offset + length); parsableByteArray.setPosition(offset); - Charset charset = detectUtfCharset(parsableByteArray); + Charset charset = detectCharset(data, offset, length); @Nullable List cuesWithTimingBeforeRequestedStartTimeUs = @@ -188,12 +202,27 @@ public void parse( } /** - * Determine UTF encoding of the byte array from a byte order mark (BOM), defaulting to UTF-8 if - * no BOM is found. + * Returns the charset to use for line parsing. + * + *

    A byte order mark takes precedence. Otherwise, input detected as anything other than UTF-8 + * or US-ASCII is transcoded to UTF-8 in {@link #parsableByteArray} first. */ - private Charset detectUtfCharset(ParsableByteArray data) { - @Nullable Charset charset = data.readUtfCharsetFromBom(); - return charset != null ? charset : StandardCharsets.UTF_8; + private Charset detectCharset(byte[] data, int offset, int length) { + @Nullable Charset utfCharset = parsableByteArray.readUtfCharsetFromBom(); + if (utfCharset != null) { + return utfCharset; + } + @Nullable + Charset detectedCharset = + charsetDetector != null ? charsetDetector.detect(data, offset, length) : null; + Charset charset = detectedCharset != null ? detectedCharset : StandardCharsets.UTF_8; + if (charset.equals(StandardCharsets.UTF_8) || charset.equals(StandardCharsets.US_ASCII)) { + return charset; + } + // Normalize detected input to UTF-8 so line parsing uses a single code path. + parsableByteArray.reset( + new String(data, offset, length, charset).getBytes(StandardCharsets.UTF_8)); + return StandardCharsets.UTF_8; } /** diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java index b8bef9fc7f3..3176deac156 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java @@ -20,11 +20,16 @@ import androidx.media3.common.Format; import androidx.media3.common.MimeTypes; +import androidx.media3.extractor.text.SubtitleParser.OutputOptions; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.base.CharMatcher; import com.google.common.collect.ImmutableList; import java.lang.reflect.Field; import java.lang.reflect.Modifier; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -32,6 +37,64 @@ @RunWith(AndroidJUnit4.class) public class DefaultSubtitleParserFactoryTest { + @Test + public void createStandaloneSubripParser_usesCharsetDetector() { + Charset charset = Charset.forName("GB18030"); + DefaultSubtitleParserFactory factory = + new DefaultSubtitleParserFactory((data, offset, length) -> charset); + Format format = new Format.Builder().setSampleMimeType(MimeTypes.APPLICATION_SUBRIP).build(); + String expectedText = "起来 快起来"; + byte[] bytes = createSubripBytes(expectedText, charset); + + List cues = new ArrayList<>(); + factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); + + assertThat(cues).hasSize(1); + assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + } + + @Test + public void createStandaloneSubripParserWithTextContainerMimeType_usesCharsetDetector() { + Charset charset = Charset.forName("GB18030"); + DefaultSubtitleParserFactory factory = + new DefaultSubtitleParserFactory((data, offset, length) -> charset); + Format format = + new Format.Builder() + .setSampleMimeType(MimeTypes.APPLICATION_SUBRIP) + .setContainerMimeType(MimeTypes.APPLICATION_SUBRIP) + .build(); + String expectedText = "起来 快起来"; + byte[] bytes = createSubripBytes(expectedText, charset); + + List cues = new ArrayList<>(); + factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); + + assertThat(cues).hasSize(1); + assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + } + + @Test + public void createEmbeddedSubripParser_doesNotUseCharsetDetector() { + DefaultSubtitleParserFactory factory = + new DefaultSubtitleParserFactory( + (data, offset, length) -> { + throw new AssertionError("Charset detector should not be called"); + }); + Format format = + new Format.Builder() + .setSampleMimeType(MimeTypes.APPLICATION_SUBRIP) + .setContainerMimeType(MimeTypes.VIDEO_MATROSKA) + .build(); + String expectedText = "This is an embedded subtitle."; + byte[] bytes = createSubripBytes(expectedText, StandardCharsets.UTF_8); + + List cues = new ArrayList<>(); + factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); + + assertThat(cues).hasSize(1); + assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + } + /** * This test loops through all the public fields of {@link MimeTypes} and assumes all the static, * string fields with a single "/" in them are MIME types - then it uses these to 'fuzz' the @@ -74,4 +137,8 @@ public void formatSupportIsConsistent() throws Exception { } } } + + private static byte[] createSubripBytes(String text, Charset charset) { + return ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + text + "\r\n").getBytes(charset); + } } diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java index b8cab4f943c..876ba224748 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java @@ -28,6 +28,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; import java.io.IOException; +import java.nio.charset.Charset; +import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import org.junit.Test; @@ -83,6 +85,89 @@ public void parseTypical() throws IOException { assertTypicalCue3(allCues.get(2)); } + @Test + public void parseGb18030WithCharsetDetector_outputsDecodedText() { + assertTextParsedWithCharsetDetector("起来 快起来", Charset.forName("GB18030")); + } + + @Test + public void parseEucKrWithCharsetDetector_outputsDecodedText() { + assertTextParsedWithCharsetDetector("이것은 한국어 자막 테스트입니다.", Charset.forName("EUC-KR")); + } + + @Test + public void parseShiftJisWithCharsetDetector_outputsDecodedText() { + assertTextParsedWithCharsetDetector("これは日本語の字幕テストです。", Charset.forName("Shift_JIS")); + } + + @Test + public void parseUtf8WithCharsetDetector_outputsDecodedText() { + assertTextParsedWithCharsetDetector("This is a UTF-8 subtitle.", StandardCharsets.UTF_8); + } + + @Test + public void parseUsAsciiWithCharsetDetector_outputsDecodedText() { + assertTextParsedWithCharsetDetector("This is an ASCII subtitle.", StandardCharsets.US_ASCII); + } + + @Test + public void parseWithByteOrderMark_doesNotCallCharsetDetector() throws IOException { + SubripParser parser = + new SubripParser( + (data, offset, length) -> { + throw new AssertionError("Charset detector should not be called"); + }); + byte[] bytes = + TestUtil.getByteArray( + ApplicationProvider.getApplicationContext(), TYPICAL_WITH_BYTE_ORDER_MARK); + + ImmutableList allCues = parseAllCues(parser, bytes); + + assertThat(allCues).hasSize(3); + assertTypicalCue1(allCues.get(0)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); + } + + @Test + public void parseWithCharsetDetectorReturningNull_defaultsToUtf8() throws IOException { + SubripParser parser = new SubripParser((data, offset, length) -> null); + byte[] bytes = TestUtil.getByteArray(ApplicationProvider.getApplicationContext(), TYPICAL_FILE); + + ImmutableList allCues = parseAllCues(parser, bytes); + + assertThat(allCues).hasSize(3); + assertTypicalCue1(allCues.get(0)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); + } + + @Test + public void parseAtOffsetWithCharsetDetector_passesRequestedRangeToDetector() { + Charset charset = Charset.forName("GB18030"); + String expectedText = "这是一个字幕测试。"; + byte[] subtitleBytes = + ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + expectedText + "\r\n").getBytes(charset); + int offset = 5; + byte[] bytes = new byte[offset + subtitleBytes.length + 7]; + System.arraycopy(subtitleBytes, 0, bytes, offset, subtitleBytes.length); + SubripParser parser = + new SubripParser( + (data, detectorOffset, detectorLength) -> { + assertThat(data).isSameInstanceAs(bytes); + assertThat(detectorOffset).isEqualTo(offset); + assertThat(detectorLength).isEqualTo(subtitleBytes.length); + return charset; + }); + ImmutableList.Builder cues = ImmutableList.builder(); + + parser.parse(bytes, offset, subtitleBytes.length, OutputOptions.allCues(), cues::add); + + ImmutableList allCues = cues.build(); + assertThat(allCues).hasSize(1); + assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + } + @Test public void parseTypicalAtOffsetAndRestrictedLength() throws IOException { SubripParser parser = new SubripParser(); @@ -309,6 +394,17 @@ private static ImmutableList parseAllCues(SubtitleParser parser, return cues.build(); } + private static void assertTextParsedWithCharsetDetector(String expectedText, Charset charset) { + byte[] bytes = + ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + expectedText + "\r\n").getBytes(charset); + SubripParser parser = new SubripParser((data, offset, length) -> charset); + + ImmutableList allCues = parseAllCues(parser, bytes); + + assertThat(allCues).hasSize(1); + assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + } + private static void assertTypicalCue1(CuesWithTiming cuesWithTiming) { assertThat(cuesWithTiming.startTimeUs).isEqualTo(0); assertThat(cuesWithTiming.cues.get(0).text.toString()).isEqualTo("This is the first subtitle."); From ec380aa2952776692ddbbd3dc740ae62412f4326 Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Tue, 22 Sep 2026 15:21:46 +0100 Subject: [PATCH 2/7] Check which charsets ParsableByteArray supports --- .../media3/common/util/ParsableByteArray.java | 9 ++++++++ .../text/DefaultSubtitleParserFactory.java | 13 ++++++----- .../extractor/text/subrip/SubripParser.java | 23 ++++++++++++------- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/libraries/common/src/main/java/androidx/media3/common/util/ParsableByteArray.java b/libraries/common/src/main/java/androidx/media3/common/util/ParsableByteArray.java index c936a14f9bc..921f01e2e34 100644 --- a/libraries/common/src/main/java/androidx/media3/common/util/ParsableByteArray.java +++ b/libraries/common/src/main/java/androidx/media3/common/util/ParsableByteArray.java @@ -16,6 +16,7 @@ package androidx.media3.common.util; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import static java.nio.ByteOrder.BIG_ENDIAN; import static java.nio.ByteOrder.LITTLE_ENDIAN; @@ -783,6 +784,14 @@ public Charset readUtfCharsetFromBom() { return null; } + /** + * Returns whether the provided {@link Charset} is supported when passed to methods like {@link + * #peekChar(Charset)} and {@link #readLine(Charset)}. + */ + public static boolean isCharsetSupported(Charset charset) { + return SUPPORTED_CHARSETS_FOR_READLINE.contains(checkNotNull(charset)); + } + /** * Sets whether all read/peek methods should enforce that {@link #getPosition()} never exceeds * {@link #limit()}. diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java index 9eaa75eedde..52e95a08d9f 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java @@ -55,19 +55,20 @@ public final class DefaultSubtitleParserFactory implements SubtitleParser.Factor @Nullable private final CharsetDetector charsetDetector; - /** Creates an instance that defaults to UTF-8 for SubRip subtitles without a byte order mark. */ + /** Creates an instance. */ public DefaultSubtitleParserFactory() { this(/* charsetDetector= */ null); } /** - * Creates an instance that uses {@code charsetDetector} for standalone SubRip subtitles without a - * byte order mark. + * Creates an instance that passes {@code charsetDetector} to delegate factories (where relevant). * - *

    The detector is not used for SubRip subtitles embedded in a media container because these - * samples may contain only a small part of the subtitle file. + *

    The detector is only passed to delegate factories that support it, and only when the charset + * may be ambiguous. For example, it won't be passed to {@link SubripParser} when handling SRT + * data extracted from a Matroska container, because the Matroska spec requires that this must be + * in UTF-8. * - * @param charsetDetector The detector to use, or {@code null} to default to UTF-8. + * @param charsetDetector The detector to use, or {@code null}. */ public DefaultSubtitleParserFactory(@Nullable CharsetDetector charsetDetector) { this.charsetDetector = charsetDetector; diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java index 16415c73c0f..77f4705e1a5 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java @@ -204,22 +204,29 @@ public void parse( /** * Returns the charset to use for line parsing. * - *

    A byte order mark takes precedence. Otherwise, input detected as anything other than UTF-8 - * or US-ASCII is transcoded to UTF-8 in {@link #parsableByteArray} first. + *

    A byte order mark takes precedence, otherwise the data is passed to {@link + * #charsetDetector}. + * + *

    If the detected charset isn't supported by {@link ParsableByteArray} then the underlying + * data is transcoded to UTF-8 before this method returns. */ private Charset detectCharset(byte[] data, int offset, int length) { @Nullable Charset utfCharset = parsableByteArray.readUtfCharsetFromBom(); if (utfCharset != null) { return utfCharset; } - @Nullable - Charset detectedCharset = - charsetDetector != null ? charsetDetector.detect(data, offset, length) : null; - Charset charset = detectedCharset != null ? detectedCharset : StandardCharsets.UTF_8; - if (charset.equals(StandardCharsets.UTF_8) || charset.equals(StandardCharsets.US_ASCII)) { + if (charsetDetector == null) { + return StandardCharsets.UTF_8; + } + @Nullable Charset charset = charsetDetector.detect(data, offset, length); + if (charset == null) { + return StandardCharsets.UTF_8; + } + if (ParsableByteArray.isCharsetSupported(charset)) { return charset; } - // Normalize detected input to UTF-8 so line parsing uses a single code path. + // ParsableByteArray doesn't support directly reading the detected charset, so we transcode + // the data to UTF-8 (which is supported). parsableByteArray.reset( new String(data, offset, length, charset).getBytes(StandardCharsets.UTF_8)); return StandardCharsets.UTF_8; From 8b20d82b4b8c36fc6929afd28d05dfb433840adf Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Tue, 22 Sep 2026 15:50:05 +0100 Subject: [PATCH 3/7] Parameterize some tests, and simplify others --- .../text/subrip/SubripParserTest.java | 181 ++++++++---------- 1 file changed, 85 insertions(+), 96 deletions(-) diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java index 876ba224748..70f81f5deb5 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java @@ -17,16 +17,27 @@ import static androidx.media3.common.Format.CUE_REPLACEMENT_BEHAVIOR_MERGE; import static com.google.common.truth.Truth.assertThat; - +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import androidx.annotation.Nullable; import androidx.media3.common.text.Cue; +import androidx.media3.extractor.text.CharsetDetector; import androidx.media3.extractor.text.CuesWithTiming; import androidx.media3.extractor.text.SubtitleParser; import androidx.media3.extractor.text.SubtitleParser.OutputOptions; import androidx.media3.test.utils.TestUtil; import androidx.test.core.app.ApplicationProvider; -import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.collect.ImmutableList; import com.google.common.collect.Iterables; +import com.google.common.primitives.Bytes; +import com.google.testing.junit.testparameterinjector.TestParameter; import java.io.IOException; import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; @@ -34,9 +45,10 @@ import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestParameterInjector; /** Unit test for {@link SubripParser}. */ -@RunWith(AndroidJUnit4.class) +@RunWith(RobolectricTestParameterInjector.class) public final class SubripParserTest { private static final String EMPTY_FILE = "media/subrip/empty"; @@ -85,89 +97,6 @@ public void parseTypical() throws IOException { assertTypicalCue3(allCues.get(2)); } - @Test - public void parseGb18030WithCharsetDetector_outputsDecodedText() { - assertTextParsedWithCharsetDetector("起来 快起来", Charset.forName("GB18030")); - } - - @Test - public void parseEucKrWithCharsetDetector_outputsDecodedText() { - assertTextParsedWithCharsetDetector("이것은 한국어 자막 테스트입니다.", Charset.forName("EUC-KR")); - } - - @Test - public void parseShiftJisWithCharsetDetector_outputsDecodedText() { - assertTextParsedWithCharsetDetector("これは日本語の字幕テストです。", Charset.forName("Shift_JIS")); - } - - @Test - public void parseUtf8WithCharsetDetector_outputsDecodedText() { - assertTextParsedWithCharsetDetector("This is a UTF-8 subtitle.", StandardCharsets.UTF_8); - } - - @Test - public void parseUsAsciiWithCharsetDetector_outputsDecodedText() { - assertTextParsedWithCharsetDetector("This is an ASCII subtitle.", StandardCharsets.US_ASCII); - } - - @Test - public void parseWithByteOrderMark_doesNotCallCharsetDetector() throws IOException { - SubripParser parser = - new SubripParser( - (data, offset, length) -> { - throw new AssertionError("Charset detector should not be called"); - }); - byte[] bytes = - TestUtil.getByteArray( - ApplicationProvider.getApplicationContext(), TYPICAL_WITH_BYTE_ORDER_MARK); - - ImmutableList allCues = parseAllCues(parser, bytes); - - assertThat(allCues).hasSize(3); - assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(1)); - assertTypicalCue3(allCues.get(2)); - } - - @Test - public void parseWithCharsetDetectorReturningNull_defaultsToUtf8() throws IOException { - SubripParser parser = new SubripParser((data, offset, length) -> null); - byte[] bytes = TestUtil.getByteArray(ApplicationProvider.getApplicationContext(), TYPICAL_FILE); - - ImmutableList allCues = parseAllCues(parser, bytes); - - assertThat(allCues).hasSize(3); - assertTypicalCue1(allCues.get(0)); - assertTypicalCue2(allCues.get(1)); - assertTypicalCue3(allCues.get(2)); - } - - @Test - public void parseAtOffsetWithCharsetDetector_passesRequestedRangeToDetector() { - Charset charset = Charset.forName("GB18030"); - String expectedText = "这是一个字幕测试。"; - byte[] subtitleBytes = - ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + expectedText + "\r\n").getBytes(charset); - int offset = 5; - byte[] bytes = new byte[offset + subtitleBytes.length + 7]; - System.arraycopy(subtitleBytes, 0, bytes, offset, subtitleBytes.length); - SubripParser parser = - new SubripParser( - (data, detectorOffset, detectorLength) -> { - assertThat(data).isSameInstanceAs(bytes); - assertThat(detectorOffset).isEqualTo(offset); - assertThat(detectorLength).isEqualTo(subtitleBytes.length); - return charset; - }); - ImmutableList.Builder cues = ImmutableList.builder(); - - parser.parse(bytes, offset, subtitleBytes.length, OutputOptions.allCues(), cues::add); - - ImmutableList allCues = cues.build(); - assertThat(allCues).hasSize(1); - assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); - } - @Test public void parseTypicalAtOffsetAndRestrictedLength() throws IOException { SubripParser parser = new SubripParser(); @@ -217,8 +146,22 @@ public void parseTypical_cuesAfterTimeThenCuesBefore() throws IOException { } @Test - public void parseTypicalWithByteOrderMark() throws IOException { - SubripParser parser = new SubripParser(); + public void parseTypical_charsetDetectorReturnsNull_assumesUtf8() throws IOException { + SubripParser parser = new SubripParser(/* charsetDetector= */ (data, offset, length) -> null); + byte[] bytes = TestUtil.getByteArray(ApplicationProvider.getApplicationContext(), TYPICAL_FILE); + + ImmutableList allCues = parseAllCues(parser, bytes); + + assertThat(allCues).hasSize(3); + assertTypicalCue1(allCues.get(0)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); + } + + @Test + public void parseTypicalWithByteOrderMark_doesNotCallCharsetDetector() throws IOException { + CharsetDetector charsetDetector = mock(CharsetDetector.class); + SubripParser parser = new SubripParser(charsetDetector); byte[] bytes = TestUtil.getByteArray( ApplicationProvider.getApplicationContext(), TYPICAL_WITH_BYTE_ORDER_MARK); @@ -229,6 +172,8 @@ public void parseTypicalWithByteOrderMark() throws IOException { assertTypicalCue1(allCues.get(0)); assertTypicalCue2(allCues.get(1)); assertTypicalCue3(allCues.get(2)); + verify(charsetDetector, never()) + .detect(/* data= */ any(), /* offset= */ anyInt(), /* length= */ anyInt()); } @Test @@ -388,21 +333,65 @@ public void parseTypicalBadTimestamps() throws IOException { assertTypicalCue1(Iterables.getOnlyElement(allCues)); } - private static ImmutableList parseAllCues(SubtitleParser parser, byte[] data) { - ImmutableList.Builder cues = ImmutableList.builder(); - parser.parse(data, OutputOptions.allCues(), cues::add); - return cues.build(); + private enum CharsetTestCase { + GB18030("起来 快起来", Charset.forName("GB18030")), + EUC_KR("이것은 한국어 자막 테스트입니다.", Charset.forName("EUC-KR")), + SHIFT_JIS("これは日本語の字幕テストです。", Charset.forName("Shift_JIS")), + UTF_8("This is a UTF-8 subtitle.", StandardCharsets.UTF_8), + US_ASCII("This is an ASCII subtitle.", StandardCharsets.US_ASCII); + + @Nullable private final Charset charset; + private final String text; + + private CharsetTestCase(String text, Charset charset) { + this.charset = charset; + this.text = text; + } } - private static void assertTextParsedWithCharsetDetector(String expectedText, Charset charset) { + @Test + public void parseWithCharsetDetector_outputsDecodedText(@TestParameter CharsetTestCase testCase) { byte[] bytes = - ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + expectedText + "\r\n").getBytes(charset); - SubripParser parser = new SubripParser((data, offset, length) -> charset); + ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + testCase.text + "\r\n") + .getBytes(testCase.charset); + SubripParser parser = + new SubripParser(/* charsetDetector= */ (data, offset, length) -> testCase.charset); ImmutableList allCues = parseAllCues(parser, bytes); assertThat(allCues).hasSize(1); - assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); + assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(testCase.text); + } + + @Test + public void parseAtOffsetWithCharsetDetector_passesRequestedRangeToDetector() { + Charset charset = Charset.forName("GB18030"); + int offset = 5; + int padding = 7; + String text = "这是一个字幕测试。"; + byte[] bytes = + Bytes.concat( + new byte[offset], + ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + text + "\r\n").getBytes(charset), + new byte[padding]); + int subtitleLength = bytes.length - offset - padding; + CharsetDetector charsetDetector = mock(CharsetDetector.class); + when(charsetDetector.detect(/* data= */ any(), /* offset= */ anyInt(), /* length= */ anyInt())) + .thenReturn(charset); + SubripParser parser = new SubripParser(charsetDetector); + + List allCues = new ArrayList<>(); + parser.parse(bytes, offset, subtitleLength, OutputOptions.allCues(), allCues::add); + + assertThat(allCues).hasSize(1); + assertThat(allCues.get(0).cues.get(0).text.toString()).isEqualTo(text); + verify(charsetDetector).detect(same(bytes), eq(offset), eq(subtitleLength)); + } + + private static ImmutableList parseAllCues(SubtitleParser parser, byte[] data) { + ImmutableList.Builder cues = ImmutableList.builder(); + parser.parse(data, OutputOptions.allCues(), cues::add); + return cues.build(); } private static void assertTypicalCue1(CuesWithTiming cuesWithTiming) { From 441ee05d76ce155fd611e585d7067746c8d6797c Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Tue, 22 Sep 2026 16:24:36 +0100 Subject: [PATCH 4/7] Revert DefaultSubtitleParserFactoryTest: These tests are re-testing too much of SubripParser behaviour --- .../DefaultSubtitleParserFactoryTest.java | 67 ------------------- 1 file changed, 67 deletions(-) diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java index 3176deac156..b8bef9fc7f3 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/DefaultSubtitleParserFactoryTest.java @@ -20,16 +20,11 @@ import androidx.media3.common.Format; import androidx.media3.common.MimeTypes; -import androidx.media3.extractor.text.SubtitleParser.OutputOptions; import androidx.test.ext.junit.runners.AndroidJUnit4; import com.google.common.base.CharMatcher; import com.google.common.collect.ImmutableList; import java.lang.reflect.Field; import java.lang.reflect.Modifier; -import java.nio.charset.Charset; -import java.nio.charset.StandardCharsets; -import java.util.ArrayList; -import java.util.List; import org.junit.Test; import org.junit.runner.RunWith; @@ -37,64 +32,6 @@ @RunWith(AndroidJUnit4.class) public class DefaultSubtitleParserFactoryTest { - @Test - public void createStandaloneSubripParser_usesCharsetDetector() { - Charset charset = Charset.forName("GB18030"); - DefaultSubtitleParserFactory factory = - new DefaultSubtitleParserFactory((data, offset, length) -> charset); - Format format = new Format.Builder().setSampleMimeType(MimeTypes.APPLICATION_SUBRIP).build(); - String expectedText = "起来 快起来"; - byte[] bytes = createSubripBytes(expectedText, charset); - - List cues = new ArrayList<>(); - factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); - - assertThat(cues).hasSize(1); - assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); - } - - @Test - public void createStandaloneSubripParserWithTextContainerMimeType_usesCharsetDetector() { - Charset charset = Charset.forName("GB18030"); - DefaultSubtitleParserFactory factory = - new DefaultSubtitleParserFactory((data, offset, length) -> charset); - Format format = - new Format.Builder() - .setSampleMimeType(MimeTypes.APPLICATION_SUBRIP) - .setContainerMimeType(MimeTypes.APPLICATION_SUBRIP) - .build(); - String expectedText = "起来 快起来"; - byte[] bytes = createSubripBytes(expectedText, charset); - - List cues = new ArrayList<>(); - factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); - - assertThat(cues).hasSize(1); - assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); - } - - @Test - public void createEmbeddedSubripParser_doesNotUseCharsetDetector() { - DefaultSubtitleParserFactory factory = - new DefaultSubtitleParserFactory( - (data, offset, length) -> { - throw new AssertionError("Charset detector should not be called"); - }); - Format format = - new Format.Builder() - .setSampleMimeType(MimeTypes.APPLICATION_SUBRIP) - .setContainerMimeType(MimeTypes.VIDEO_MATROSKA) - .build(); - String expectedText = "This is an embedded subtitle."; - byte[] bytes = createSubripBytes(expectedText, StandardCharsets.UTF_8); - - List cues = new ArrayList<>(); - factory.create(format).parse(bytes, OutputOptions.allCues(), cues::add); - - assertThat(cues).hasSize(1); - assertThat(cues.get(0).cues.get(0).text.toString()).isEqualTo(expectedText); - } - /** * This test loops through all the public fields of {@link MimeTypes} and assumes all the static, * string fields with a single "/" in them are MIME types - then it uses these to 'fuzz' the @@ -137,8 +74,4 @@ public void formatSupportIsConsistent() throws Exception { } } } - - private static byte[] createSubripBytes(String text, Charset charset) { - return ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + text + "\r\n").getBytes(charset); - } } From c0e288878c9634753ce0842948d735f81a0c4bee Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Tue, 22 Sep 2026 16:36:23 +0100 Subject: [PATCH 5/7] make some small fixes in the test in response to review feedback --- .../media3/extractor/text/subrip/SubripParserTest.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java index 70f81f5deb5..9565e999d63 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java @@ -26,7 +26,6 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import androidx.annotation.Nullable; import androidx.media3.common.text.Cue; import androidx.media3.extractor.text.CharsetDetector; import androidx.media3.extractor.text.CuesWithTiming; @@ -340,7 +339,7 @@ private enum CharsetTestCase { UTF_8("This is a UTF-8 subtitle.", StandardCharsets.UTF_8), US_ASCII("This is an ASCII subtitle.", StandardCharsets.US_ASCII); - @Nullable private final Charset charset; + private final Charset charset; private final String text; private CharsetTestCase(String text, Charset charset) { @@ -350,7 +349,8 @@ private CharsetTestCase(String text, Charset charset) { } @Test - public void parseWithCharsetDetector_outputsDecodedText(@TestParameter CharsetTestCase testCase) { + public void parse_withCharsetDetector_outputsDecodedText( + @TestParameter CharsetTestCase testCase) { byte[] bytes = ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + testCase.text + "\r\n") .getBytes(testCase.charset); @@ -364,7 +364,7 @@ public void parseWithCharsetDetector_outputsDecodedText(@TestParameter CharsetTe } @Test - public void parseAtOffsetWithCharsetDetector_passesRequestedRangeToDetector() { + public void parse_atOffsetWithCharsetDetector_passesRequestedRangeToDetector() { Charset charset = Charset.forName("GB18030"); int offset = 5; int padding = 7; From 63a53f9f72f561d29887bf1642dd3cf95bbe19e2 Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Fri, 25 Sep 2026 09:49:43 +0100 Subject: [PATCH 6/7] Split transcoding out into a separate method to make it more clear --- .../extractor/text/subrip/SubripParser.java | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java index 77f4705e1a5..f68c6299bc9 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java @@ -117,7 +117,13 @@ public void parse( Consumer output) { parsableByteArray.reset(data, /* limit= */ offset + length); parsableByteArray.setPosition(offset); - Charset charset = detectCharset(data, offset, length); + Charset charset = detectCharset(); + if (!ParsableByteArray.isCharsetSupported(charset)) { + // ParsableByteArray doesn't support directly reading the detected charset, so we transcode + // the data to UTF-8 (which is supported). + transcodeToUtf8(charset); + charset = StandardCharsets.UTF_8; + } @Nullable List cuesWithTimingBeforeRequestedStartTimeUs = @@ -202,7 +208,7 @@ public void parse( } /** - * Returns the charset to use for line parsing. + * Returns the charset to use for line parsing of {@link #parsableByteArray}. * *

    A byte order mark takes precedence, otherwise the data is passed to {@link * #charsetDetector}. @@ -210,7 +216,7 @@ public void parse( *

    If the detected charset isn't supported by {@link ParsableByteArray} then the underlying * data is transcoded to UTF-8 before this method returns. */ - private Charset detectCharset(byte[] data, int offset, int length) { + private Charset detectCharset() { @Nullable Charset utfCharset = parsableByteArray.readUtfCharsetFromBom(); if (utfCharset != null) { return utfCharset; @@ -218,18 +224,24 @@ private Charset detectCharset(byte[] data, int offset, int length) { if (charsetDetector == null) { return StandardCharsets.UTF_8; } - @Nullable Charset charset = charsetDetector.detect(data, offset, length); - if (charset == null) { - return StandardCharsets.UTF_8; - } - if (ParsableByteArray.isCharsetSupported(charset)) { - return charset; - } - // ParsableByteArray doesn't support directly reading the detected charset, so we transcode - // the data to UTF-8 (which is supported). + @Nullable + Charset charset = + charsetDetector.detect( + parsableByteArray.getData(), + parsableByteArray.getPosition(), + parsableByteArray.bytesLeft()); + return charset != null ? charset : StandardCharsets.UTF_8; + } + + /** Transcodes the data behind {@link #parsableByteArray} from {@code sourceCharset} to UTF-8. */ + private void transcodeToUtf8(Charset sourceCharset) { parsableByteArray.reset( - new String(data, offset, length, charset).getBytes(StandardCharsets.UTF_8)); - return StandardCharsets.UTF_8; + new String( + parsableByteArray.getData(), + parsableByteArray.getPosition(), + parsableByteArray.bytesLeft(), + sourceCharset) + .getBytes(StandardCharsets.UTF_8)); } /** From 7f8e6cdeb5af5701ed92c25ab67dd15e5bfeafe2 Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Fri, 25 Sep 2026 10:06:26 +0100 Subject: [PATCH 7/7] Move 'is matroska' check into SubripParser --- .../text/DefaultSubtitleParserFactory.java | 14 +---- .../extractor/text/subrip/SubripParser.java | 51 +++++++++------- .../text/subrip/SubripParserTest.java | 61 +++++++++++++++++-- 3 files changed, 87 insertions(+), 39 deletions(-) diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java index 52e95a08d9f..60ac5a90bf8 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/DefaultSubtitleParserFactory.java @@ -46,9 +46,6 @@ *

  • DVB ({@link DvbParser}) *
  • TTML ({@link TtmlParser}) * - * - *

    A {@link CharsetDetector} can be provided to detect the character encoding of standalone - * SubRip subtitles without a byte order mark. */ @UnstableApi public final class DefaultSubtitleParserFactory implements SubtitleParser.Factory { @@ -63,11 +60,6 @@ public DefaultSubtitleParserFactory() { /** * Creates an instance that passes {@code charsetDetector} to delegate factories (where relevant). * - *

    The detector is only passed to delegate factories that support it, and only when the charset - * may be ambiguous. For example, it won't be passed to {@link SubripParser} when handling SRT - * data extracted from a Matroska container, because the Matroska spec requires that this must be - * in UTF-8. - * * @param charsetDetector The detector to use, or {@code null}. */ public DefaultSubtitleParserFactory(@Nullable CharsetDetector charsetDetector) { @@ -130,7 +122,7 @@ public SubtitleParser create(Format format) { case MimeTypes.APPLICATION_MP4VTT: return new Mp4WebvttParser(); case MimeTypes.APPLICATION_SUBRIP: - return new SubripParser(isStandaloneSubrip(format) ? charsetDetector : null); + return new SubripParser(charsetDetector, format); case MimeTypes.APPLICATION_TX3G: return new Tx3gParser(format.initializationData); case MimeTypes.APPLICATION_PGS: @@ -147,8 +139,4 @@ public SubtitleParser create(Format format) { } throw new IllegalArgumentException("Unsupported MIME type: " + mimeType); } - - private static boolean isStandaloneSubrip(Format format) { - return format.containerMimeType == null || MimeTypes.isText(format.containerMimeType); - } } diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java index f68c6299bc9..77be5ce7679 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/subrip/SubripParser.java @@ -26,6 +26,7 @@ import androidx.media3.common.C; import androidx.media3.common.Format; import androidx.media3.common.Format.CueReplacementBehavior; +import androidx.media3.common.MimeTypes; import androidx.media3.common.text.Cue; import androidx.media3.common.util.Consumer; import androidx.media3.common.util.Log; @@ -84,23 +85,27 @@ public final class SubripParser implements SubtitleParser { private final ArrayList tags; private final ParsableByteArray parsableByteArray; @Nullable private final CharsetDetector charsetDetector; + private final Format format; + /** Creates an instance. */ public SubripParser() { - this(/* charsetDetector= */ null); + this(/* charsetDetector= */ null, new Format.Builder().build()); } /** - * Creates an instance that uses {@code charsetDetector} when the input doesn't contain a byte - * order mark. + * Creates an instance that uses {@code charsetDetector} when the charset of the input is + * ambiguous. * - * @param charsetDetector The detector to use, or {@code null} to default to UTF-8 when the input - * doesn't contain a byte order mark. + * @param charsetDetector The detector to use, or {@code null} to default to UTF-8 when the + * charset of the input can't be determined by other means. + * @param format The format of the subrip track. */ - public SubripParser(@Nullable CharsetDetector charsetDetector) { + public SubripParser(@Nullable CharsetDetector charsetDetector, Format format) { textBuilder = new StringBuilder(); tags = new ArrayList<>(); parsableByteArray = new ParsableByteArray(); this.charsetDetector = charsetDetector; + this.format = format; } @Override @@ -121,7 +126,13 @@ public void parse( if (!ParsableByteArray.isCharsetSupported(charset)) { // ParsableByteArray doesn't support directly reading the detected charset, so we transcode // the data to UTF-8 (which is supported). - transcodeToUtf8(charset); + parsableByteArray.reset( + new String( + parsableByteArray.getData(), + parsableByteArray.getPosition(), + parsableByteArray.bytesLeft(), + charset) + .getBytes(StandardCharsets.UTF_8)); charset = StandardCharsets.UTF_8; } @@ -210,13 +221,20 @@ public void parse( /** * Returns the charset to use for line parsing of {@link #parsableByteArray}. * - *

    A byte order mark takes precedence, otherwise the data is passed to {@link - * #charsetDetector}. + *

    The result is derived from, in order: * - *

    If the detected charset isn't supported by {@link ParsableByteArray} then the underlying - * data is transcoded to UTF-8 before this method returns. + *

      + *
    1. Content from a Matroska container is assumed to always be UTF-8 (per spec). + *
    2. A byte order mark in the data, if present. + *
    3. The result of calling {@link #charsetDetector}, if non-null. + *
    4. Fallback to assuming UTF-8. + *
    */ private Charset detectCharset() { + if (MimeTypes.isMatroska(format.containerMimeType)) { + // Subrip muxed into a Matroska container is always UTF-8. + return StandardCharsets.UTF_8; + } @Nullable Charset utfCharset = parsableByteArray.readUtfCharsetFromBom(); if (utfCharset != null) { return utfCharset; @@ -233,17 +251,6 @@ private Charset detectCharset() { return charset != null ? charset : StandardCharsets.UTF_8; } - /** Transcodes the data behind {@link #parsableByteArray} from {@code sourceCharset} to UTF-8. */ - private void transcodeToUtf8(Charset sourceCharset) { - parsableByteArray.reset( - new String( - parsableByteArray.getData(), - parsableByteArray.getPosition(), - parsableByteArray.bytesLeft(), - sourceCharset) - .getBytes(StandardCharsets.UTF_8)); - } - /** * Trims and removes tags from the given line. The removed tags are added to {@code tags}. * diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java index 9565e999d63..6ef7d5a6ac9 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/subrip/SubripParserTest.java @@ -26,6 +26,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import androidx.media3.common.Format; +import androidx.media3.common.MimeTypes; import androidx.media3.common.text.Cue; import androidx.media3.extractor.text.CharsetDetector; import androidx.media3.extractor.text.CuesWithTiming; @@ -67,6 +69,9 @@ public final class SubripParserTest { "media/subrip/typical_no_hours_and_millis"; private static final String TYPICAL_BAD_TIMESTAMPS = "media/subrip/typical_bad_timestamps"; + private static final Format STANDALONE_SUBRIP_FORMAT = + new Format.Builder().setContainerMimeType(MimeTypes.APPLICATION_SUBRIP).build(); + @Test public void cueReplacementBehaviorIsMerge() { SubripParser parser = new SubripParser(); @@ -146,7 +151,9 @@ public void parseTypical_cuesAfterTimeThenCuesBefore() throws IOException { @Test public void parseTypical_charsetDetectorReturnsNull_assumesUtf8() throws IOException { - SubripParser parser = new SubripParser(/* charsetDetector= */ (data, offset, length) -> null); + SubripParser parser = + new SubripParser( + /* charsetDetector= */ (data, offset, length) -> null, STANDALONE_SUBRIP_FORMAT); byte[] bytes = TestUtil.getByteArray(ApplicationProvider.getApplicationContext(), TYPICAL_FILE); ImmutableList allCues = parseAllCues(parser, bytes); @@ -160,7 +167,29 @@ public void parseTypical_charsetDetectorReturnsNull_assumesUtf8() throws IOExcep @Test public void parseTypicalWithByteOrderMark_doesNotCallCharsetDetector() throws IOException { CharsetDetector charsetDetector = mock(CharsetDetector.class); - SubripParser parser = new SubripParser(charsetDetector); + SubripParser parser = new SubripParser(charsetDetector, STANDALONE_SUBRIP_FORMAT); + byte[] bytes = + TestUtil.getByteArray( + ApplicationProvider.getApplicationContext(), TYPICAL_WITH_BYTE_ORDER_MARK); + + ImmutableList allCues = parseAllCues(parser, bytes); + + assertThat(allCues).hasSize(3); + assertTypicalCue1(allCues.get(0)); + assertTypicalCue2(allCues.get(1)); + assertTypicalCue3(allCues.get(2)); + verify(charsetDetector, never()) + .detect(/* data= */ any(), /* offset= */ anyInt(), /* length= */ anyInt()); + } + + @Test + public void parseTypicalWithByteOrderMark_inMkvContainer_doesNotCallCharsetDetector() + throws IOException { + CharsetDetector charsetDetector = mock(CharsetDetector.class); + SubripParser parser = + new SubripParser( + charsetDetector, + new Format.Builder().setContainerMimeType(MimeTypes.VIDEO_MATROSKA).build()); byte[] bytes = TestUtil.getByteArray( ApplicationProvider.getApplicationContext(), TYPICAL_WITH_BYTE_ORDER_MARK); @@ -262,6 +291,28 @@ public void parseTypicalUtf16LittleEndian() throws IOException { assertTypicalCue3(allCues.get(2)); } + @Test + public void parseTypicalUtf16LittleEndian_inMkv_doesNotCallCharsetDetector_producesGarbage() + throws IOException { + CharsetDetector charsetDetector = mock(CharsetDetector.class); + SubripParser parser = + new SubripParser( + charsetDetector, + new Format.Builder().setContainerMimeType(MimeTypes.VIDEO_MATROSKA).build()); + byte[] bytes = + TestUtil.getByteArray(ApplicationProvider.getApplicationContext(), TYPICAL_UTF16LE); + + ImmutableList _ = parseAllCues(parser, bytes); + + // Don't assert the specific output, since it's effectively undefined behaviour. This test + // exists to assert we don't call CharsetDetector for every subtitle sample from a container + // where the charset is defined by the spec - because most CharsetDetector implementations + // are statistical (and so rely on longer documents than a single subtitle) and also potentially + // slow (so we don't want to call it for every text sample). + verify(charsetDetector, never()) + .detect(/* data= */ any(), /* offset= */ anyInt(), /* length= */ anyInt()); + } + @Test public void parseTypicalUtf16BigEndian() throws IOException { SubripParser parser = new SubripParser(); @@ -355,7 +406,9 @@ public void parse_withCharsetDetector_outputsDecodedText( ("1\r\n" + "00:00:00,000 --> 00:00:05,000\r\n" + testCase.text + "\r\n") .getBytes(testCase.charset); SubripParser parser = - new SubripParser(/* charsetDetector= */ (data, offset, length) -> testCase.charset); + new SubripParser( + /* charsetDetector= */ (data, offset, length) -> testCase.charset, + STANDALONE_SUBRIP_FORMAT); ImmutableList allCues = parseAllCues(parser, bytes); @@ -378,7 +431,7 @@ public void parse_atOffsetWithCharsetDetector_passesRequestedRangeToDetector() { CharsetDetector charsetDetector = mock(CharsetDetector.class); when(charsetDetector.detect(/* data= */ any(), /* offset= */ anyInt(), /* length= */ anyInt())) .thenReturn(charset); - SubripParser parser = new SubripParser(charsetDetector); + SubripParser parser = new SubripParser(charsetDetector, STANDALONE_SUBRIP_FORMAT); List allCues = new ArrayList<>(); parser.parse(bytes, offset, subtitleLength, OutputOptions.allCues(), allCues::add);