From e6448f34983f58188e97e08976f219efb10e0178 Mon Sep 17 00:00:00 2001
From: ibaker <ibaker@google.com>
Date: Thu, 14 Nov 2024 11:01:33 -0800
Subject: [PATCH] Don't assume MP4 keyframe metadata is correct for CEA
 re-ordering

The content in Issue: androidx/media#1863 has every sample incorrectly marked as a
sync sample in the MP4 metadata, which results in flushing the
re-ordering queue on every sample, so nothing gets re-ordered, so the
subtitles are garbled.

There are currently two "uses" for this call on every keyframe:
1. It offers a safety valve if we don't read a `maxNumReorderSamples`
value from the video. Without this, the queue will just keep growing
and end up swallowing all subtitle data (similar to the bug fixed by
https://github.com/androidx/media/commit/39c734963f67f8ed3fa79820d573e6b3297c8d28).

2. When we do read (or infer) a `maxNumReorderSamples` it means we can
emit samples from the queue slightly earlier - but this is pretty
marginal, given i think the max possible value for
`maxNumReorderSamples` is 16, so the most benefit we would get is 16
frames (~0.53s at 30fps) - in most cases we will have more than 0.5s
of buffer ahead of the playback position, so the subtitles will still
get shown at the right time with no problem.

(1) is resolved in this change by setting the queue size to zero (no
reordering) if we don't have a value for `maxNumReorderSamples`.

(2) has minimal impact, so we just accept it.

We may be able to inspect the NAL unit to determine IDR vs non-IDR
instead - we will consider this as a follow-up change, but given the
minimal impact of (2) we may not pursue this.

#cherrypick

PiperOrigin-RevId: 696583702
---
 RELEASENOTES.md                                    |  3 +++
 .../extractor/mp4/FragmentedMp4Extractor.java      | 14 +++++++-------
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/RELEASENOTES.md b/RELEASENOTES.md
index de912f643d..acb595a6d8 100644
--- a/RELEASENOTES.md
+++ b/RELEASENOTES.md
@@ -47,6 +47,9 @@
         per sample.
     *   Fix playback hanging on DASH multi-period streams when CEA-608 subtitles
         are enabled ([#1863](https://github.com/androidx/media/issues/1863)).
+    *   Fix garbled CEA-608 subtitles in MP4 files that incorrectly mark every
+        sample as a sync sample
+        ([#1863](https://github.com/androidx/media/issues/1863)).
 *   Metadata:
 *   Image:
 *   DRM:
diff --git a/libraries/extractor/src/main/java/androidx/media3/extractor/mp4/FragmentedMp4Extractor.java b/libraries/extractor/src/main/java/androidx/media3/extractor/mp4/FragmentedMp4Extractor.java
index 104b54ce66..3f8dd29b53 100644
--- a/libraries/extractor/src/main/java/androidx/media3/extractor/mp4/FragmentedMp4Extractor.java
+++ b/libraries/extractor/src/main/java/androidx/media3/extractor/mp4/FragmentedMp4Extractor.java
@@ -1643,17 +1643,17 @@ public class FragmentedMp4Extractor implements Extractor {
                     : 0);
             nalBuffer.setLimit(unescapedLength);
 
-            if (track.format.maxNumReorderSamples != Format.NO_VALUE
-                && track.format.maxNumReorderSamples != reorderingSeiMessageQueue.getMaxSize()) {
+            if (track.format.maxNumReorderSamples == Format.NO_VALUE) {
+              if (reorderingSeiMessageQueue.getMaxSize() != 0) {
+                reorderingSeiMessageQueue.setMaxSize(0);
+              }
+            } else if (reorderingSeiMessageQueue.getMaxSize()
+                != track.format.maxNumReorderSamples) {
               reorderingSeiMessageQueue.setMaxSize(track.format.maxNumReorderSamples);
             }
             reorderingSeiMessageQueue.add(sampleTimeUs, nalBuffer);
 
-            boolean sampleIsKeyFrameOrEndOfStream =
-                (trackBundle.getCurrentSampleFlags()
-                        & (C.BUFFER_FLAG_KEY_FRAME | C.BUFFER_FLAG_END_OF_STREAM))
-                    != 0;
-            if (sampleIsKeyFrameOrEndOfStream) {
+            if ((trackBundle.getCurrentSampleFlags() & C.BUFFER_FLAG_END_OF_STREAM) != 0) {
               reorderingSeiMessageQueue.flush();
             }
           } else {