From 3b7ec52ca876a4a095451393400b94aba1b16a45 Mon Sep 17 00:00:00 2001 From: datdoantelus Date: Thu, 25 Apr 2024 12:32:25 -0400 Subject: [PATCH 1/6] Ignore rowLock and numLock as define in CTA-708 spec. Update current row value when new line is added. --- .../java/androidx/media3/extractor/text/cea/Cea708Decoder.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java index 077d1e2210..c75bf57336 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java @@ -1228,6 +1228,8 @@ public final class Cea708Decoder extends CeaDecoder { || rolledUpCaptions.size() >= MAXIMUM_ROW_COUNT) { rolledUpCaptions.remove(0); } + // update row value after newline + row = rolledUpCaptions.size(); } else { captionStringBuilder.append(text); } From ec3065e98119060bb424ec04a97faae636db8312 Mon Sep 17 00:00:00 2001 From: datdoantelus Date: Tue, 21 May 2024 08:40:28 -0400 Subject: [PATCH 2/6] revert rowLock and colomnLock changes since it will be done separately --- .../java/androidx/media3/extractor/text/cea/Cea708Decoder.java | 1 + 1 file changed, 1 insertion(+) diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java index c75bf57336..975e85ea2c 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java @@ -353,6 +353,7 @@ public final class Cea708Decoder extends CeaDecoder { int endBlockPosition = captionChannelPacketData.getPosition() + (blockSize * 8); while (captionChannelPacketData.getPosition() < endBlockPosition) { int command = captionChannelPacketData.readBits(8); + Log.d("DatDoan", "Cea708 handleCommand " + command); if (command != COMMAND_EXT1) { if (command <= GROUP_C0_END) { handleC0Command(command); From 327d26cb325919162c1b3c6ec18d7b77660c7202 Mon Sep 17 00:00:00 2001 From: datdoantelus Date: Tue, 21 May 2024 08:42:24 -0400 Subject: [PATCH 3/6] remove test comment --- .../java/androidx/media3/extractor/text/cea/Cea708Decoder.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java index 975e85ea2c..9345b0ea2f 100644 --- a/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java +++ b/libraries/extractor/src/main/java/androidx/media3/extractor/text/cea/Cea708Decoder.java @@ -353,7 +353,6 @@ public final class Cea708Decoder extends CeaDecoder { int endBlockPosition = captionChannelPacketData.getPosition() + (blockSize * 8); while (captionChannelPacketData.getPosition() < endBlockPosition) { int command = captionChannelPacketData.readBits(8); - Log.d("DatDoan", "Cea708 handleCommand " + command); if (command != COMMAND_EXT1) { if (command <= GROUP_C0_END) { handleC0Command(command); @@ -779,11 +778,9 @@ public final class Cea708Decoder extends CeaDecoder { // first byte captionChannelPacketData.skipBits(2); // null padding boolean visible = captionChannelPacketData.readBit(); - // ANSI/CTA-708-E S-2023 spec (Section 8.4.7) indicates that rowLock and columnLock values in // the media should be ignored and assumed to be true. captionChannelPacketData.skipBits(2); - int priority = captionChannelPacketData.readBits(3); // second byte boolean relativePositioning = captionChannelPacketData.readBit(); From ed5d47e856dbf2debf0b36007c991d93a0384b88 Mon Sep 17 00:00:00 2001 From: datdoantelus Date: Wed, 22 May 2024 11:58:57 -0400 Subject: [PATCH 4/6] Add cea708Decoder test for setPenLocation command and newline handling --- .../extractor/text/cea/Cea708DecoderTest.java | 70 +++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java index f4b196c6ec..19f26c8a57 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java @@ -118,6 +118,76 @@ public class Cea708DecoderTest { assertThat(getOnlyCue(result).text.toString()).isEqualTo("row3\nrow4"); } + @Test + public void setPenLocationTestWithNewLineTest1() throws Exception { + Cea708Decoder cea708Decoder = + new Cea708Decoder( + + /* accessibilityChannel= */ Format.NO_VALUE, /* initializationData= */ null); + byte[] windowDefinition = + TestUtil.createByteArray( + 0x98, // DF0 command (define window 0) + 0b0010_0000, // visible=true, row lock and column lock disabled, priority=0 + 0xF0 | 50, // relative positioning, anchor vertical + 50, // anchor horizontal + 10, // anchor point = 0, row count = 10 + 30, // column count = 30 + 0b0000_1001); // window style = 1, pen style = 1 + byte[] setCurrentWindow = TestUtil.createByteArray(0x80); // CW0 (set current window to 0) + byte[] setPenLocation = TestUtil.createByteArray(0x92,0x01,0x00); // COMMAND_SPL with row 1 and + byte[] newLine = TestUtil.createByteArray(0x0D); // new line + byte[] subtitleData = + encodePacketIntoBytePairs( + createPacket( + /* sequenceNumber= */ 0, + createServiceBlock( + Bytes.concat( + windowDefinition, + setCurrentWindow, + "line1".getBytes(Charsets.UTF_8), + newLine, + setPenLocation, + "line2".getBytes(Charsets.UTF_8))))); + + Subtitle firstSubtitle = decodeSampleAndCopyResult(cea708Decoder, subtitleData); + + assertThat(getOnlyCue(firstSubtitle).text.toString()).isEqualTo("line1\nline2"); + } + + @Test + public void setPenLocationTestWithNewLineTest2() throws Exception { + Cea708Decoder cea708Decoder = + new Cea708Decoder( + + /* accessibilityChannel= */ Format.NO_VALUE, /* initializationData= */ null); + byte[] windowDefinition = + TestUtil.createByteArray( + 0x98, // DF0 command (define window 0) + 0b0010_0000, // visible=true, row lock and column lock disabled, priority=0 + 0xF0 | 50, // relative positioning, anchor vertical + 50, // anchor horizontal + 10, // anchor point = 0, row count = 10 + 30, // column count = 30 + 0b0000_1001); // window style = 1, pen style = 1 + byte[] setCurrentWindow = TestUtil.createByteArray(0x80); // CW0 (set current window to 0) + byte[] setPenLocation = TestUtil.createByteArray(0x92,0x01,0x00); // COMMAND_SPL with row 1 and + byte[] subtitleData = + encodePacketIntoBytePairs( + createPacket( + /* sequenceNumber= */ 0, + createServiceBlock( + Bytes.concat( + windowDefinition, + setCurrentWindow, + "line1".getBytes(Charsets.UTF_8), + setPenLocation, + "line2".getBytes(Charsets.UTF_8))))); + + Subtitle firstSubtitle = decodeSampleAndCopyResult(cea708Decoder, subtitleData); + + assertThat(getOnlyCue(firstSubtitle).text.toString()).isEqualTo("line1\nline2"); + } + /** * Queues {@code sample} to {@code decoder} and dequeues the result, then copies and returns it if * it's non-null. From 3aebb38cd36daf3c5f50f8d0e468062aa5a27a5a Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Thu, 23 May 2024 15:20:30 +0100 Subject: [PATCH 5/6] Rename and reshuffle tests --- .../extractor/text/cea/Cea708DecoderTest.java | 87 +++++++++++-------- 1 file changed, 51 insertions(+), 36 deletions(-) diff --git a/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java b/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java index 19f26c8a57..28deb05c8d 100644 --- a/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java +++ b/libraries/extractor/src/test/java/androidx/media3/extractor/text/cea/Cea708DecoderTest.java @@ -118,8 +118,14 @@ public class Cea708DecoderTest { assertThat(getOnlyCue(result).text.toString()).isEqualTo("row3\nrow4"); } + /** + * ExoPlayer's incomplete implementation of the 'set pen location' command appends a newline if + * the 'new' row location is different to the 'current' row (this is to ensure that subtitles that + * are meant to be on different lines aren't concatenated together on a single line). This test + * demonstrates this, even though the target row is 2, only a single newline is appended. + */ @Test - public void setPenLocationTestWithNewLineTest1() throws Exception { + public void setPenLocation_appendsNewlineIfRowChanges() throws Exception { Cea708Decoder cea708Decoder = new Cea708Decoder( @@ -134,7 +140,50 @@ public class Cea708DecoderTest { 30, // column count = 30 0b0000_1001); // window style = 1, pen style = 1 byte[] setCurrentWindow = TestUtil.createByteArray(0x80); // CW0 (set current window to 0) - byte[] setPenLocation = TestUtil.createByteArray(0x92,0x01,0x00); // COMMAND_SPL with row 1 and + // COMMAND_SPL with row 2 and column 0 + byte[] setPenLocation = TestUtil.createByteArray(0x92, 0x02, 0x00); + byte[] subtitleData = + encodePacketIntoBytePairs( + createPacket( + /* sequenceNumber= */ 0, + createServiceBlock( + Bytes.concat( + windowDefinition, + setCurrentWindow, + "line1".getBytes(Charsets.UTF_8), + setPenLocation, + "line2".getBytes(Charsets.UTF_8))))); + + Subtitle firstSubtitle = decodeSampleAndCopyResult(cea708Decoder, subtitleData); + + assertThat(getOnlyCue(firstSubtitle).text.toString()).isEqualTo("line1\nline2"); + } + + /** + * ExoPlayer's incomplete implementation of the 'set pen location' command appends a newline if + * the 'new' row location is different to the 'current' row (this is to ensure that subtitles that + * are meant to be on different lines aren't concatenated together on a single line). This test + * ensures that if there's already an explicit newline appended before the command, a duplicate + * newline isn't appended. + */ + @Test + public void setPenLocation_explicitNewLineBefore_secondNewlineNotAdded() throws Exception { + Cea708Decoder cea708Decoder = + new Cea708Decoder( + + /* accessibilityChannel= */ Format.NO_VALUE, /* initializationData= */ null); + byte[] windowDefinition = + TestUtil.createByteArray( + 0x98, // DF0 command (define window 0) + 0b0010_0000, // visible=true, row lock and column lock disabled, priority=0 + 0xF0 | 50, // relative positioning, anchor vertical + 50, // anchor horizontal + 10, // anchor point = 0, row count = 10 + 30, // column count = 30 + 0b0000_1001); // window style = 1, pen style = 1 + byte[] setCurrentWindow = TestUtil.createByteArray(0x80); // CW0 (set current window to 0) + // COMMAND_SPL with row 1 and column 0 + byte[] setPenLocation = TestUtil.createByteArray(0x92, 0x01, 0x00); byte[] newLine = TestUtil.createByteArray(0x0D); // new line byte[] subtitleData = encodePacketIntoBytePairs( @@ -154,40 +203,6 @@ public class Cea708DecoderTest { assertThat(getOnlyCue(firstSubtitle).text.toString()).isEqualTo("line1\nline2"); } - @Test - public void setPenLocationTestWithNewLineTest2() throws Exception { - Cea708Decoder cea708Decoder = - new Cea708Decoder( - - /* accessibilityChannel= */ Format.NO_VALUE, /* initializationData= */ null); - byte[] windowDefinition = - TestUtil.createByteArray( - 0x98, // DF0 command (define window 0) - 0b0010_0000, // visible=true, row lock and column lock disabled, priority=0 - 0xF0 | 50, // relative positioning, anchor vertical - 50, // anchor horizontal - 10, // anchor point = 0, row count = 10 - 30, // column count = 30 - 0b0000_1001); // window style = 1, pen style = 1 - byte[] setCurrentWindow = TestUtil.createByteArray(0x80); // CW0 (set current window to 0) - byte[] setPenLocation = TestUtil.createByteArray(0x92,0x01,0x00); // COMMAND_SPL with row 1 and - byte[] subtitleData = - encodePacketIntoBytePairs( - createPacket( - /* sequenceNumber= */ 0, - createServiceBlock( - Bytes.concat( - windowDefinition, - setCurrentWindow, - "line1".getBytes(Charsets.UTF_8), - setPenLocation, - "line2".getBytes(Charsets.UTF_8))))); - - Subtitle firstSubtitle = decodeSampleAndCopyResult(cea708Decoder, subtitleData); - - assertThat(getOnlyCue(firstSubtitle).text.toString()).isEqualTo("line1\nline2"); - } - /** * Queues {@code sample} to {@code decoder} and dequeues the result, then copies and returns it if * it's non-null. From e87b820626b3bd6a4c21d48b27100ce0fe23a1ab Mon Sep 17 00:00:00 2001 From: Ian Baker Date: Thu, 23 May 2024 15:24:28 +0100 Subject: [PATCH 6/6] Add release note --- RELEASENOTES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index 883862e386..64a3f7a43a 100644 --- a/RELEASENOTES.md +++ b/RELEASENOTES.md @@ -507,6 +507,9 @@ This release includes the following changes since the `rowLock` and `columnLock` should both be assumed to be true, regardless of the values present in the stream (`columnLock` support is not implemented, so it's effectively assumed to always be false). + * CEA-708: Avoid duplicate newlines being added by ExoPlayer's naive + handling of the 'set pen location' command + ([#1315](https://github.com/androidx/media/pull/1315)). * Image: * Add support for DASH thumbnails. Grid images are cropped and individual thumbnails are provided to `ImageOutput` close to their presentation