From 27a4f96cb17b727935f85e5a8c99e766b0711c72 Mon Sep 17 00:00:00 2001 From: "Venkatarama NG. Avadhani" Date: Tue, 30 Jul 2019 11:47:33 +0530 Subject: [PATCH] Clean up FLAC picture parsing --- .../exoplayer2/ext/flac/FlacExtractor.java | 4 +- extensions/flac/src/main/jni/flac_jni.cc | 48 ++++++++----------- extensions/flac/src/main/jni/flac_parser.cc | 30 +++++++----- .../flac/src/main/jni/include/flac_parser.h | 14 +++--- .../metadata/flac/PictureFrame.java | 8 ++-- .../exoplayer2/util/FlacStreamMetadata.java | 12 ++--- ...PictureTest.java => PictureFrameTest.java} | 2 +- .../util/FlacStreamMetadataTest.java | 8 ++-- .../android/exoplayer2/ui/PlayerView.java | 21 +++++--- 9 files changed, 76 insertions(+), 71 deletions(-) rename library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/{PictureTest.java => PictureFrameTest.java} (97%) diff --git a/extensions/flac/src/main/java/com/google/android/exoplayer2/ext/flac/FlacExtractor.java b/extensions/flac/src/main/java/com/google/android/exoplayer2/ext/flac/FlacExtractor.java index 9f79f09117..cd91b06288 100644 --- a/extensions/flac/src/main/java/com/google/android/exoplayer2/ext/flac/FlacExtractor.java +++ b/extensions/flac/src/main/java/com/google/android/exoplayer2/ext/flac/FlacExtractor.java @@ -229,8 +229,8 @@ public final class FlacExtractor implements Extractor { binarySearchSeeker = outputSeekMap(decoderJni, streamMetadata, input.getLength(), extractorOutput); 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); outputBuffer.reset(streamMetadata.maxDecodedFrameSize()); diff --git a/extensions/flac/src/main/jni/flac_jni.cc b/extensions/flac/src/main/jni/flac_jni.cc index 4ccd24781b..9cc611559a 100644 --- a/extensions/flac/src/main/jni/flac_jni.cc +++ b/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 pictures = context->parser->getPictures(); - jclass flacPictureFrameClass = env->FindClass( + jobject jPictures = env->NewObject(arrayListClass, arrayListConstructor); + bool picturesValid = context->parser->arePicturesValid(); + if (picturesValid) { + std::vector pictures = context->parser->getPictures(); + jclass pictureFrameClass = env->FindClass( "com/google/android/exoplayer2/metadata/flac/PictureFrame"); - jmethodID flacPictureFrameConstructor = env->GetMethodID( - flacPictureFrameClass, "", + jmethodID pictureFrameConstructor = env->GetMethodID( + pictureFrameClass, "", "(ILjava/lang/String;Ljava/lang/String;IIII[B)V"); - for (std::vector::const_iterator picture = pictures.begin(); + for (std::vector::const_iterator picture = pictures.begin(); picture != pictures.end(); ++picture) { jstring mimeType = env->NewStringUTF(picture->mimeType.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]); - jobject flacPictureFrame = env->NewObject(flacPictureFrameClass, - flacPictureFrameConstructor, + jobject pictureFrame = env->NewObject(pictureFrameClass, + pictureFrameConstructor, picture->type, mimeType, description, @@ -142,11 +142,11 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) { picture->height, picture->depth, picture->colors, - picArr); - env->CallBooleanMethod(pictureList, arrayListAddMethod, flacPictureFrame); + pictureData); + env->CallBooleanMethod(jPictures, arrayListAddMethod, pictureFrame); env->DeleteLocalRef(mimeType); env->DeleteLocalRef(description); - env->DeleteLocalRef(picArr); + env->DeleteLocalRef(pictureData); } } @@ -160,18 +160,12 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) { flacStreamMetadataClass, "", "(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) { diff --git a/extensions/flac/src/main/jni/flac_parser.cc b/extensions/flac/src/main/jni/flac_parser.cc index fafc254482..b9e5cace71 100644 --- a/extensions/flac/src/main/jni/flac_parser.cc +++ b/extensions/flac/src/main/jni/flac_parser.cc @@ -193,18 +193,22 @@ void FLACParser::metadataCallback(const FLAC__StreamMetadata *metadata) { break; 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; } default: @@ -269,7 +273,7 @@ FLACParser::FLACParser(DataSource *source) mEOF(false), mStreamInfoValid(false), mVorbisCommentsValid(false), - mPicValid(false), + mPicturesValid(false), mWriteRequested(false), mWriteCompleted(false), mWriteBuffer(NULL), diff --git a/extensions/flac/src/main/jni/include/flac_parser.h b/extensions/flac/src/main/jni/include/flac_parser.h index f1d175b94f..9c6452c160 100644 --- a/extensions/flac/src/main/jni/include/flac_parser.h +++ b/extensions/flac/src/main/jni/include/flac_parser.h @@ -30,7 +30,7 @@ typedef int status_t; -typedef struct { +struct FlacPicture { int type; std::string mimeType; std::string description; @@ -39,7 +39,7 @@ typedef struct { FLAC__uint32 depth; FLAC__uint32 colors; std::vector data; -} flacPicture; +}; class FLACParser { public: @@ -65,9 +65,9 @@ class FLACParser { return mVorbisComments; } - bool isPicValid() const { return mPicValid; } + bool arePicturesValid() const { return mPicturesValid; } - const std::vector& getPictures() const { return mPictures; } + const std::vector& getPictures() const { return mPictures; } int64_t getLastFrameTimestamp() const { return (1000000LL * mWriteHeader.number.sample_number) / getSampleRate(); @@ -97,7 +97,7 @@ class FLACParser { if (newPosition == 0) { mStreamInfoValid = false; mVorbisCommentsValid = false; - mPicValid = false; + mPicturesValid = false; mVorbisComments.clear(); mPictures.clear(); FLAC__stream_decoder_reset(mDecoder); @@ -150,8 +150,8 @@ class FLACParser { bool mVorbisCommentsValid; // cached when the PICTURE metadata is parsed by libFLAC - std::vector mPictures; - bool mPicValid; + std::vector mPictures; + bool mPicturesValid; // cached when a decoded PCM block is "written" by libFLAC parser bool mWriteRequested; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/metadata/flac/PictureFrame.java b/library/core/src/main/java/com/google/android/exoplayer2/metadata/flac/PictureFrame.java index fcf1fd6e58..dc280be9ff 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/metadata/flac/PictureFrame.java +++ b/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. */ public final String mimeType; /** A description of the picture. */ - @Nullable public final String description; + public final String description; /** The pixel width of the picture. */ public final int width; /** The pixel height of the picture. */ @@ -49,7 +49,7 @@ public final class PictureFrame implements Metadata.Entry { public PictureFrame( int pictureType, String mimeType, - @Nullable String description, + String description, int width, int height, int depth, @@ -111,8 +111,8 @@ public final class PictureFrame implements Metadata.Entry { public int hashCode() { int result = 17; 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 + height; result = 31 * result + depth; diff --git a/library/core/src/main/java/com/google/android/exoplayer2/util/FlacStreamMetadata.java b/library/core/src/main/java/com/google/android/exoplayer2/util/FlacStreamMetadata.java index 48680b5095..e7851aa0a4 100644 --- a/library/core/src/main/java/com/google/android/exoplayer2/util/FlacStreamMetadata.java +++ b/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 bitsPerSample; public final long totalSamples; - @Nullable public final Metadata flacMetadata; + @Nullable public final Metadata metadata; private static final String SEPARATOR = "="; @@ -59,7 +59,7 @@ public final class FlacStreamMetadata { this.channels = scratch.readBits(3) + 1; this.bitsPerSample = scratch.readBits(5) + 1; 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 totalSamples Total samples of the FLAC stream. * @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 FLAC format * METADATA_BLOCK_STREAMINFO * @see FLAC format @@ -90,7 +90,7 @@ public final class FlacStreamMetadata { int bitsPerSample, long totalSamples, List vorbisComments, - List pictureList) { + List pictures) { this.minBlockSize = minBlockSize; this.maxBlockSize = maxBlockSize; this.minFrameSize = minFrameSize; @@ -99,8 +99,8 @@ public final class FlacStreamMetadata { this.channels = channels; this.bitsPerSample = bitsPerSample; 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. */ diff --git a/library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureTest.java b/library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureFrameTest.java similarity index 97% rename from library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureTest.java rename to library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureFrameTest.java index 04a5b46e26..4263103eeb 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/metadata/flac/PictureTest.java +++ b/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}. */ @RunWith(AndroidJUnit4.class) -public class PictureTest { +public final class PictureFrameTest { @Test public void testParcelable() { diff --git a/library/core/src/test/java/com/google/android/exoplayer2/util/FlacStreamMetadataTest.java b/library/core/src/test/java/com/google/android/exoplayer2/util/FlacStreamMetadataTest.java index c556282ca2..3988e5e45e 100644 --- a/library/core/src/test/java/com/google/android/exoplayer2/util/FlacStreamMetadataTest.java +++ b/library/core/src/test/java/com/google/android/exoplayer2/util/FlacStreamMetadataTest.java @@ -36,7 +36,7 @@ public final class FlacStreamMetadataTest { Metadata metadata = new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>()) - .flacMetadata; + .metadata; assertThat(metadata.length()).isEqualTo(2); VorbisComment commentFrame = (VorbisComment) metadata.get(0); @@ -53,7 +53,7 @@ public final class FlacStreamMetadataTest { Metadata metadata = new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>()) - .flacMetadata; + .metadata; assertThat(metadata).isNull(); } @@ -65,7 +65,7 @@ public final class FlacStreamMetadataTest { Metadata metadata = new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>()) - .flacMetadata; + .metadata; assertThat(metadata.length()).isEqualTo(1); VorbisComment commentFrame = (VorbisComment) metadata.get(0); @@ -81,7 +81,7 @@ public final class FlacStreamMetadataTest { Metadata metadata = new FlacStreamMetadata(0, 0, 0, 0, 0, 0, 0, 0, commentsList, new ArrayList<>()) - .flacMetadata; + .metadata; assertThat(metadata.length()).isEqualTo(1); VorbisComment commentFrame = (VorbisComment) metadata.get(0); diff --git a/library/ui/src/main/java/com/google/android/exoplayer2/ui/PlayerView.java b/library/ui/src/main/java/com/google/android/exoplayer2/ui/PlayerView.java index 4ac007fa55..1abdd33bb2 100644 --- a/library/ui/src/main/java/com/google/android/exoplayer2/ui/PlayerView.java +++ b/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 boolean isTouching; private static final int PICTURE_TYPE_FRONT_COVER = 3; + private static final int PICTURE_TYPE_NOT_SET = -1; public PlayerView(Context context) { this(context, null); @@ -1249,25 +1250,31 @@ public class PlayerView extends FrameLayout implements AdsLoader.AdViewProvider private boolean setArtworkFromMetadata(Metadata metadata) { boolean isArtworkSet = false; - int currentPicType = -1; + int currentPictureType = PICTURE_TYPE_NOT_SET; for (int i = 0; i < metadata.length(); i++) { Metadata.Entry metadataEntry = metadata.get(i); - int picType; + int pictureType; byte[] bitmapData; if (metadataEntry instanceof ApicFrame) { bitmapData = ((ApicFrame) metadataEntry).pictureData; - picType = ((ApicFrame) metadataEntry).pictureType; + pictureType = ((ApicFrame) metadataEntry).pictureType; } else if (metadataEntry instanceof PictureFrame) { bitmapData = ((PictureFrame) metadataEntry).pictureData; - picType = ((PictureFrame) metadataEntry).pictureType; + pictureType = ((PictureFrame) metadataEntry).pictureType; } else { 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); 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;