Clean up FLAC picture parsing

extensions/flac/src/main/java/com/google/android/exoplayer2/ext/flac/FlacExtractor.java

@@ -229,8 +229,8 @@ public final class FlacExtractor implements Extractor {
       binarySearchSeeker =
       binarySearchSeeker =
           outputSeekMap(decoderJni, streamMetadata, input.getLength(), extractorOutput);
           outputSeekMap(decoderJni, streamMetadata, input.getLength(), extractorOutput);
       Metadata metadata = id3MetadataDisabled ? null : id3Metadata;
       Metadata metadata = id3MetadataDisabled ? null : id3Metadata;
-      if (streamMetadata.flacMetadata != null) {
-        metadata = streamMetadata.flacMetadata.copyWithAppendedEntriesFrom(metadata);
+      if (streamMetadata.metadata != null) {
+        metadata = streamMetadata.metadata.copyWithAppendedEntriesFrom(metadata);
       }
       }
       outputFormat(streamMetadata, metadata, trackOutput);
       outputFormat(streamMetadata, metadata, trackOutput);
       outputBuffer.reset(streamMetadata.maxDecodedFrameSize());
       outputBuffer.reset(streamMetadata.maxDecodedFrameSize());

extensions/flac/src/main/jni/flac_jni.cc

@@ -117,24 +117,24 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) {
     }
     }
   }
   }
 
 
-  jobject pictureList = env->NewObject(arrayListClass, arrayListConstructor);
-  bool picValid = context->parser->isPicValid();
-  if (picValid) {
-    std::vector<flacPicture> pictures = context->parser->getPictures();
-    jclass flacPictureFrameClass = env->FindClass(
+  jobject jPictures = env->NewObject(arrayListClass, arrayListConstructor);
+  bool picturesValid = context->parser->arePicturesValid();
+  if (picturesValid) {
+    std::vector<FlacPicture> pictures = context->parser->getPictures();
+    jclass pictureFrameClass = env->FindClass(
         "com/google/android/exoplayer2/metadata/flac/PictureFrame");
         "com/google/android/exoplayer2/metadata/flac/PictureFrame");
-    jmethodID flacPictureFrameConstructor = env->GetMethodID(
-        flacPictureFrameClass, "<init>",
+    jmethodID pictureFrameConstructor = env->GetMethodID(
+        pictureFrameClass, "<init>",
         "(ILjava/lang/String;Ljava/lang/String;IIII[B)V");
         "(ILjava/lang/String;Ljava/lang/String;IIII[B)V");
-    for (std::vector<flacPicture>::const_iterator picture = pictures.begin();
+    for (std::vector<FlacPicture>::const_iterator picture = pictures.begin();
                   picture != pictures.end(); ++picture) {
                   picture != pictures.end(); ++picture) {
       jstring mimeType = env->NewStringUTF(picture->mimeType.c_str());
       jstring mimeType = env->NewStringUTF(picture->mimeType.c_str());
       jstring description = env->NewStringUTF(picture->description.c_str());
       jstring description = env->NewStringUTF(picture->description.c_str());
-      jbyteArray picArr = env->NewByteArray(picture->data.size());
-      env->SetByteArrayRegion(picArr, 0, picture->data.size(),
+      jbyteArray pictureData = env->NewByteArray(picture->data.size());
+      env->SetByteArrayRegion(pictureData, 0, picture->data.size(),
                               (signed char *)&picture->data[0]);
                               (signed char *)&picture->data[0]);
-      jobject flacPictureFrame = env->NewObject(flacPictureFrameClass,
-                                                flacPictureFrameConstructor,
+      jobject pictureFrame = env->NewObject(pictureFrameClass,
+                                                pictureFrameConstructor,
                                                 picture->type,
                                                 picture->type,
                                                 mimeType,
                                                 mimeType,
                                                 description,
                                                 description,
@@ -142,11 +142,11 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) {
                                                 picture->height,
                                                 picture->height,
                                                 picture->depth,
                                                 picture->depth,
                                                 picture->colors,
                                                 picture->colors,
-                                                picArr);
-      env->CallBooleanMethod(pictureList, arrayListAddMethod, flacPictureFrame);
+                                                pictureData);
+      env->CallBooleanMethod(jPictures, arrayListAddMethod, pictureFrame);
       env->DeleteLocalRef(mimeType);
       env->DeleteLocalRef(mimeType);
       env->DeleteLocalRef(description);
       env->DeleteLocalRef(description);
-      env->DeleteLocalRef(picArr);
+      env->DeleteLocalRef(pictureData);
     }
     }
   }
   }
 
 
@@ -160,18 +160,12 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) {
       flacStreamMetadataClass, "<init>",
       flacStreamMetadataClass, "<init>",
       "(IIIIIIIJLjava/util/List;Ljava/util/List;)V");
       "(IIIIIIIJLjava/util/List;Ljava/util/List;)V");
 
 
-  jobject streamMetaData = env->NewObject(flacStreamMetadataClass,
-                                          flacStreamMetadataConstructor,
-                                          streamInfo.min_blocksize,
-                                          streamInfo.max_blocksize,
-                                          streamInfo.min_framesize,
-                                          streamInfo.max_framesize,
-                                          streamInfo.sample_rate,
-                                          streamInfo.channels,
-                                          streamInfo.bits_per_sample,
-                                          streamInfo.total_samples,
-                                          commentList, pictureList);
-  return streamMetaData;
+  return env->NewObject(flacStreamMetadataClass, flacStreamMetadataConstructor,
+                        streamInfo.min_blocksize, streamInfo.max_blocksize,
+                        streamInfo.min_framesize, streamInfo.max_framesize,
+                        streamInfo.sample_rate, streamInfo.channels,
+                        streamInfo.bits_per_sample, streamInfo.total_samples,
+                        commentList, jPictures);
 }
 }
 
 
 DECODER_FUNC(jint, flacDecodeToBuffer, jlong jContext, jobject jOutputBuffer) {
 DECODER_FUNC(jint, flacDecodeToBuffer, jlong jContext, jobject jOutputBuffer) {

extensions/flac/src/main/jni/flac_parser.cc

@@ -193,18 +193,22 @@ void FLACParser::metadataCallback(const FLAC__StreamMetadata *metadata) {
       break;
       break;
     case FLAC__METADATA_TYPE_PICTURE:
     case FLAC__METADATA_TYPE_PICTURE:
       {
       {
-        const FLAC__StreamMetadata_Picture *pic = &metadata->data.picture;
-        flacPicture picture;
-        picture.mimeType.assign(std::string(pic->mime_type));
-        picture.description.assign(std::string((char *)pic->description));
-        picture.data.assign(pic->data, pic->data + pic->data_length);
-        picture.width = pic->width;
-        picture.height = pic->height;
-        picture.depth = pic->depth;
-        picture.colors = pic->colors;
-        picture.type = pic->type;
-        mPictures.push_back(picture);
-        mPicValid = true;
+        const FLAC__StreamMetadata_Picture *parsedPicture =
+                  &metadata->data.picture;
+        FlacPicture flacPicture;
+        flacPicture.mimeType.assign(std::string(parsedPicture->mime_type));
+        flacPicture.description.assign(
+            std::string((char *)parsedPicture->description));
+        flacPicture.data.assign(
+            parsedPicture->data,
+            parsedPicture->data + parsedPicture->data_length);
+        flacPicture.width = parsedPicture->width;
+        flacPicture.height = parsedPicture->height;
+        flacPicture.depth = parsedPicture->depth;
+        flacPicture.colors = parsedPicture->colors;
+        flacPicture.type = parsedPicture->type;
+        mPictures.push_back(flacPicture);
+        mPicturesValid = true;
         break;
         break;
       }
       }
     default:
     default:
@@ -269,7 +273,7 @@ FLACParser::FLACParser(DataSource *source)
       mEOF(false),
       mEOF(false),
       mStreamInfoValid(false),
       mStreamInfoValid(false),
       mVorbisCommentsValid(false),
       mVorbisCommentsValid(false),
-      mPicValid(false),
+      mPicturesValid(false),
       mWriteRequested(false),
       mWriteRequested(false),
       mWriteCompleted(false),
       mWriteCompleted(false),
       mWriteBuffer(NULL),
       mWriteBuffer(NULL),

extensions/flac/src/main/jni/include/flac_parser.h

@@ -30,7 +30,7 @@
 
 
 typedef int status_t;
 typedef int status_t;
 
 
-typedef struct {
+struct FlacPicture {
   int type;
   int type;
   std::string mimeType;
   std::string mimeType;
   std::string description;
   std::string description;
@@ -39,7 +39,7 @@ typedef struct {
   FLAC__uint32 depth;
   FLAC__uint32 depth;
   FLAC__uint32 colors;
   FLAC__uint32 colors;
   std::vector<char> data;
   std::vector<char> data;
-} flacPicture;
+};
 
 
 class FLACParser {
 class FLACParser {
  public:
  public:
@@ -65,9 +65,9 @@ class FLACParser {
     return mVorbisComments;
     return mVorbisComments;
   }
   }
 
 
-  bool isPicValid() const { return mPicValid; }
+  bool arePicturesValid() const { return mPicturesValid; }
 
 
-  const std::vector<flacPicture>& getPictures() const { return mPictures; }
+  const std::vector<FlacPicture>& getPictures() const { return mPictures; }
 
 
   int64_t getLastFrameTimestamp() const {
   int64_t getLastFrameTimestamp() const {
     return (1000000LL * mWriteHeader.number.sample_number) / getSampleRate();
     return (1000000LL * mWriteHeader.number.sample_number) / getSampleRate();
@@ -97,7 +97,7 @@ class FLACParser {
       if (newPosition == 0) {
       if (newPosition == 0) {
         mStreamInfoValid = false;
         mStreamInfoValid = false;
         mVorbisCommentsValid = false;
         mVorbisCommentsValid = false;
-        mPicValid = false;
+        mPicturesValid = false;
         mVorbisComments.clear();
         mVorbisComments.clear();
         mPictures.clear();
         mPictures.clear();
         FLAC__stream_decoder_reset(mDecoder);
         FLAC__stream_decoder_reset(mDecoder);
@@ -150,8 +150,8 @@ class FLACParser {
   bool mVorbisCommentsValid;
   bool mVorbisCommentsValid;
 
 
   // cached when the PICTURE metadata is parsed by libFLAC
   // cached when the PICTURE metadata is parsed by libFLAC
-  std::vector<flacPicture> mPictures;
-  bool mPicValid;
+  std::vector<FlacPicture> mPictures;
+  bool mPicturesValid;
 
 
   // cached when a decoded PCM block is "written" by libFLAC parser
   // cached when a decoded PCM block is "written" by libFLAC parser
   bool mWriteRequested;
   bool mWriteRequested;

library/core/src/main/java/com/google/android/exoplayer2/metadata/flac/PictureFrame.java

@@ -31,7 +31,7 @@ public final class PictureFrame implements Metadata.Entry {
   /** The mime type of the picture. */
   /** The mime type of the picture. */
   public final String mimeType;
   public final String mimeType;
   /** A description of the picture. */
   /** A description of the picture. */
-  @Nullable public final String description;
+  public final String description;
   /** The pixel width of the picture. */
   /** The pixel width of the picture. */
   public final int width;
   public final int width;
   /** The pixel height of the picture. */
   /** The pixel height of the picture. */
@@ -49,7 +49,7 @@ public final class PictureFrame implements Metadata.Entry {
   public PictureFrame(
   public PictureFrame(
       int pictureType,
       int pictureType,
       String mimeType,
       String mimeType,
-      @Nullable String description,
+      String description,
       int width,
       int width,
       int height,
       int height,
       int depth,
       int depth,
@@ -111,8 +111,8 @@ public final class PictureFrame implements Metadata.Entry {
   public int hashCode() {
   public int hashCode() {
     int result = 17;
     int result = 17;
     result = 31 * result + pictureType;
     result = 31 * result + pictureType;
-    result = 31 * result + (mimeType != null ? mimeType.hashCode() : 0);
-    result = 31 * result + (description != null ? description.hashCode() : 0);
+    result = 31 * result + mimeType.hashCode();
+    result = 31 * result + description.hashCode();
     result = 31 * result + width;
     result = 31 * result + width;
     result = 31 * result + height;
     result = 31 * result + height;
     result = 31 * result + depth;
     result = 31 * result + depth;

library/core/src/main/java/com/google/android/exoplayer2/util/FlacStreamMetadata.java

@@ -36,7 +36,7 @@ public final class FlacStreamMetadata {
   public final int channels;
   public final int channels;
   public final int bitsPerSample;
   public final int bitsPerSample;
   public final long totalSamples;
   public final long totalSamples;
-  @Nullable public final Metadata flacMetadata;
+  @Nullable public final Metadata metadata;
 
 
   private static final String SEPARATOR = "=";
   private static final String SEPARATOR = "=";
 
 
@@ -59,7 +59,7 @@ public final class FlacStreamMetadata {
     this.channels = scratch.readBits(3) + 1;
     this.channels = scratch.readBits(3) + 1;
     this.bitsPerSample = scratch.readBits(5) + 1;
     this.bitsPerSample = scratch.readBits(5) + 1;
     this.totalSamples = ((scratch.readBits(4) & 0xFL) << 32) | (scratch.readBits(32) & 0xFFFFFFFFL);
     this.totalSamples = ((scratch.readBits(4) & 0xFL) << 32) | (scratch.readBits(32) & 0xFFFFFFFFL);
-    this.flacMetadata = null;
+    this.metadata = null;
   }
   }
 
 
   /**
   /**
@@ -72,7 +72,7 @@ public final class FlacStreamMetadata {
    * @param bitsPerSample Number of bits per sample of the FLAC stream.
    * @param bitsPerSample Number of bits per sample of the FLAC stream.
    * @param totalSamples Total samples of the FLAC stream.
    * @param totalSamples Total samples of the FLAC stream.
    * @param vorbisComments Vorbis comments. Each entry must be in key=value form.
    * @param vorbisComments Vorbis comments. Each entry must be in key=value form.
-   * @param pictureList A list of pictures in the stream.
+   * @param pictures A list of pictures in the stream.
    * @see <a href="https://xiph.org/flac/format.html#metadata_block_streaminfo">FLAC format
    * @see <a href="https://xiph.org/flac/format.html#metadata_block_streaminfo">FLAC format
    *     METADATA_BLOCK_STREAMINFO</a>
    *     METADATA_BLOCK_STREAMINFO</a>
    * @see <a href="https://xiph.org/flac/format.html#metadata_block_vorbis_comment">FLAC format
    * @see <a href="https://xiph.org/flac/format.html#metadata_block_vorbis_comment">FLAC format
@@ -90,7 +90,7 @@ public final class FlacStreamMetadata {
       int bitsPerSample,
       int bitsPerSample,
       long totalSamples,
       long totalSamples,
       List<String> vorbisComments,
       List<String> vorbisComments,
-      List<PictureFrame> pictureList) {
+      List<PictureFrame> pictures) {
     this.minBlockSize = minBlockSize;
     this.minBlockSize = minBlockSize;
     this.maxBlockSize = maxBlockSize;
     this.maxBlockSize = maxBlockSize;
     this.minFrameSize = minFrameSize;
     this.minFrameSize = minFrameSize;
@@ -99,8 +99,8 @@ public final class FlacStreamMetadata {
     this.channels = channels;
     this.channels = channels;
     this.bitsPerSample = bitsPerSample;
     this.bitsPerSample = bitsPerSample;
     this.totalSamples = totalSamples;
     this.totalSamples = totalSamples;
-    Metadata metadata = new Metadata(pictureList);
-    this.flacMetadata = metadata.copyWithAppendedEntriesFrom(parseVorbisComments(vorbisComments));
+    this.metadata =
+        new Metadata(pictures).copyWithAppendedEntriesFrom(parseVorbisComments(vorbisComments));
   }
   }
 
 
   /** Returns the maximum size for a decoded frame from the FLAC stream. */
   /** Returns the maximum size for a decoded frame from the FLAC stream. */

library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureTest.java → library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureFrameTest.java

@@ -24,7 +24,7 @@ import org.junit.runner.RunWith;
 
 
 /** Test for {@link PictureFrame}. */
 /** Test for {@link PictureFrame}. */
 @RunWith(AndroidJUnit4.class)
 @RunWith(AndroidJUnit4.class)
-public class PictureTest {
+public final class PictureFrameTest {
 
 
   @Test
   @Test
   public void testParcelable() {
   public void testParcelable() {

library/core/src/test/java/com/google/android/exoplayer2/util/FlacStreamMetadataTest.java

@@ -36,7 +36,7 @@ public final class FlacStreamMetadataTest {
 
 
     Metadata metadata =
     Metadata metadata =
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
-            .flacMetadata;
+            .metadata;
 
 
     assertThat(metadata.length()).isEqualTo(2);
     assertThat(metadata.length()).isEqualTo(2);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);
@@ -53,7 +53,7 @@ public final class FlacStreamMetadataTest {
 
 
     Metadata metadata =
     Metadata metadata =
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
-            .flacMetadata;
+            .metadata;
 
 
     assertThat(metadata).isNull();
     assertThat(metadata).isNull();
   }
   }
@@ -65,7 +65,7 @@ public final class FlacStreamMetadataTest {
 
 
     Metadata metadata =
     Metadata metadata =
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
-            .flacMetadata;
+            .metadata;
 
 
     assertThat(metadata.length()).isEqualTo(1);
     assertThat(metadata.length()).isEqualTo(1);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);
@@ -81,7 +81,7 @@ public final class FlacStreamMetadataTest {
 
 
     Metadata metadata =
     Metadata metadata =
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
         new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>())
-            .flacMetadata;
+            .metadata;
 
 
     assertThat(metadata.length()).isEqualTo(1);
     assertThat(metadata.length()).isEqualTo(1);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);
     VorbisComment commentFrame = (VorbisComment) metadata.get(0);

library/ui/src/main/java/com/google/android/exoplayer2/ui/PlayerView.java

@@ -305,6 +305,7 @@ public class PlayerView extends FrameLayout implements AdsLoader.AdViewProvider
   private int textureViewRotation;
   private int textureViewRotation;
   private boolean isTouching;
   private boolean isTouching;
   private static final int PICTURE_TYPE_FRONT_COVER = 3;
   private static final int PICTURE_TYPE_FRONT_COVER = 3;
+  private static final int PICTURE_TYPE_NOT_SET = -1;
 
 
   public PlayerView(Context context) {
   public PlayerView(Context context) {
     this(context, null);
     this(context, null);
@@ -1249,25 +1250,31 @@ public class PlayerView extends FrameLayout implements AdsLoader.AdViewProvider
 
 
   private boolean setArtworkFromMetadata(Metadata metadata) {
   private boolean setArtworkFromMetadata(Metadata metadata) {
     boolean isArtworkSet = false;
     boolean isArtworkSet = false;
-    int currentPicType = -1;
+    int currentPictureType = PICTURE_TYPE_NOT_SET;
     for (int i = 0; i < metadata.length(); i++) {
     for (int i = 0; i < metadata.length(); i++) {
       Metadata.Entry metadataEntry = metadata.get(i);
       Metadata.Entry metadataEntry = metadata.get(i);
-      int picType;
+      int pictureType;
       byte[] bitmapData;
       byte[] bitmapData;
       if (metadataEntry instanceof ApicFrame) {
       if (metadataEntry instanceof ApicFrame) {
         bitmapData = ((ApicFrame) metadataEntry).pictureData;
         bitmapData = ((ApicFrame) metadataEntry).pictureData;
-        picType = ((ApicFrame) metadataEntry).pictureType;
+        pictureType = ((ApicFrame) metadataEntry).pictureType;
       } else if (metadataEntry instanceof PictureFrame) {
       } else if (metadataEntry instanceof PictureFrame) {
         bitmapData = ((PictureFrame) metadataEntry).pictureData;
         bitmapData = ((PictureFrame) metadataEntry).pictureData;
-        picType = ((PictureFrame) metadataEntry).pictureType;
+        pictureType = ((PictureFrame) metadataEntry).pictureType;
       } else {
       } else {
         continue;
         continue;
       }
       }
-      /* Prefers the first front cover picture in the picture list */
-      if (currentPicType != PICTURE_TYPE_FRONT_COVER) {
+      /* Prefers the first front cover picture.
+       * If there are no front cover pictures, prefer the first picture in the list
+       * */
+      if (currentPictureType == PICTURE_TYPE_NOT_SET || pictureType == PICTURE_TYPE_FRONT_COVER) {
         Bitmap bitmap = BitmapFactory.decodeByteArray(bitmapData, 0, bitmapData.length);
         Bitmap bitmap = BitmapFactory.decodeByteArray(bitmapData, 0, bitmapData.length);
         isArtworkSet = setDrawableArtwork(new BitmapDrawable(getResources(), bitmap));
         isArtworkSet = setDrawableArtwork(new BitmapDrawable(getResources(), bitmap));
-        currentPicType = picType;
+        currentPictureType = pictureType;
+        if (currentPictureType == PICTURE_TYPE_FRONT_COVER) {
+          /* Found a front cover, stop looking for more pictures. */
+          break;
+        }
       }
       }
     }
     }
     return isArtworkSet;
     return isArtworkSet;