Rework OPUS comment header parsing

Simplify how the comment header is parsed and eliminate a few possible
bugs in the process, such as:
- Metadata being overwritten directly by the comments header.
- The packet being rewound to 0 if it cannot find a comment header,
which might result in the cursor being moved to a bad position.

library/extractor/src/main/java/com/google/android/exoplayer2/extractor/ogg/OpusReader.java

@@ -20,9 +20,11 @@ import static com.google.android.exoplayer2.util.Assertions.checkNotNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.Nullable;
 
 
 import com.google.android.exoplayer2.Format;
 import com.google.android.exoplayer2.Format;
+import com.google.android.exoplayer2.ParserException;
 import com.google.android.exoplayer2.audio.OpusUtil;
 import com.google.android.exoplayer2.audio.OpusUtil;
 import com.google.android.exoplayer2.extractor.VorbisUtil;
 import com.google.android.exoplayer2.extractor.VorbisUtil;
 import com.google.android.exoplayer2.metadata.Metadata;
 import com.google.android.exoplayer2.metadata.Metadata;
+import com.google.android.exoplayer2.util.Log;
 import com.google.android.exoplayer2.util.MimeTypes;
 import com.google.android.exoplayer2.util.MimeTypes;
 import com.google.android.exoplayer2.util.ParsableByteArray;
 import com.google.android.exoplayer2.util.ParsableByteArray;
 
 
@@ -34,18 +36,20 @@ import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
 /** {@link StreamReader} to extract Opus data out of Ogg byte stream. */
 /** {@link StreamReader} to extract Opus data out of Ogg byte stream. */
 /* package */ final class OpusReader extends StreamReader {
 /* package */ final class OpusReader extends StreamReader {
 
 
-  private static final byte[] OPUS_ID_SIGNATURE = {'O', 'p', 'u', 's', 'H', 'e', 'a', 'd'};
-  private static final byte[] OPUS_TAG_SIGNATURE = {'O', 'p', 'u', 's', 'T', 'a', 'g', 's'};
+  private static final byte[] OPUS_ID_HEADER_SIGNATURE =
+      {'O', 'p', 'u', 's', 'H', 'e', 'a', 'd'};
+  private static final byte[] OPUS_COMMENT_HEADER_SIGNATURE =
+      {'O', 'p', 'u', 's', 'T', 'a', 'g', 's'};
 
 
   private boolean headerRead;
   private boolean headerRead;
 
 
   public static boolean verifyBitstreamType(ParsableByteArray data) {
   public static boolean verifyBitstreamType(ParsableByteArray data) {
-    if (data.bytesLeft() < OPUS_ID_SIGNATURE.length) {
+    if (data.bytesLeft() < OPUS_ID_HEADER_SIGNATURE.length) {
       return false;
       return false;
     }
     }
-    byte[] header = new byte[OPUS_ID_SIGNATURE.length];
-    data.readBytes(header, 0, OPUS_ID_SIGNATURE.length);
-    return Arrays.equals(header, OPUS_ID_SIGNATURE);
+    byte[] header = new byte[OPUS_ID_HEADER_SIGNATURE.length];
+    data.readBytes(header, 0, OPUS_ID_HEADER_SIGNATURE.length);
+    return Arrays.equals(header, OPUS_ID_HEADER_SIGNATURE);
   }
   }
 
 
   @Override
   @Override
@@ -80,20 +84,15 @@ import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
       headerRead = true;
       headerRead = true;
       return true;
       return true;
     } else {
     } else {
-      checkNotNull(setupData.format); // Has been set when the header was read.
+      checkNotNull(setupData.format); // Has been set when the header was read
 
 
-      // Handle the comments frame if we are working with one.
-      boolean isComment;
+      int startPosition = packet.getPosition();
 
 
-      if (packet.bytesLeft() < OPUS_TAG_SIGNATURE.length) {
-        isComment = false;
-      } else {
-        byte[] header = new byte[OPUS_TAG_SIGNATURE.length];
-        packet.readBytes(header, 0, OPUS_TAG_SIGNATURE.length);
-        isComment = Arrays.equals(header, OPUS_TAG_SIGNATURE);
+      if (!validateCommentHeaderSignature(packet)) {
+        packet.setPosition(startPosition);
+        return false;
       }
       }
 
 
-      if (isComment) {
       VorbisUtil.CommentHeader commentHeader = VorbisUtil.readVorbisCommentHeader(
       VorbisUtil.CommentHeader commentHeader = VorbisUtil.readVorbisCommentHeader(
           packet, false, false);
           packet, false, false);
 
 
@@ -103,15 +102,31 @@ import org.checkerframework.checker.nullness.qual.EnsuresNonNullIf;
 
 
       setupData.format = setupData.format
       setupData.format = setupData.format
           .buildUpon()
           .buildUpon()
-            .setMetadata(vorbisMetadata)
+          .setMetadata(
+              setupData.format.metadata != null
+                  ? setupData.format.metadata.copyWithAppendedEntriesFrom(vorbisMetadata)
+                  : vorbisMetadata)
           .build();
           .build();
 
 
       return true;
       return true;
-      } else {
-        packet.setPosition(0);
-        return false;
     }
     }
   }
   }
+
+  /**
+   * Validates that the given scratch signifies a comment header.
+   *
+   * @param scratch The packet data
+   * @return True if the packet signifies a comment header, false if not
+   */
+  private boolean validateCommentHeaderSignature(ParsableByteArray scratch) {
+    if (scratch.bytesLeft() < OPUS_COMMENT_HEADER_SIGNATURE.length) {
+      return false;
+    }
+
+    byte[] header = new byte[OPUS_COMMENT_HEADER_SIGNATURE.length];
+    scratch.readBytes(header, 0, OPUS_COMMENT_HEADER_SIGNATURE.length);
+
+    return Arrays.equals(header, OPUS_COMMENT_HEADER_SIGNATURE);
   }
   }
 
 
   /**
   /**