Add vorbis comments support to flac extractor #6151

vavadhani · 2019-07-09T12:11:57Z

Decode and add vorbis comments from the flac file to metadata.

Decode and add vorbis comments from the flac file to metadata. google#5527

googlebot · 2019-07-09T12:12:01Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

vavadhani · 2019-07-09T13:50:03Z

I signed it

ojw28 · 2019-07-09T16:01:16Z

@vavadhani - Please ensure you're registered under your company's Google CLA (this is distinct from the Android CLA). Thanks!

harishdm · 2019-07-09T21:54:44Z

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

+        const FLAC__StreamMetadata_VorbisComment *vc =
+            &metadata->data.vorbis_comment;
+        mVorbisCommentValid = true;
+        mVorbisComment.metadataArray =


These aren't freed anywhere.
Instead of malloc and copy, can we hold reference to &vc->comments[i]; and create required strings in flac_jni.cc flacDecodeVorbisComment()?
i.e. getVorbisComment() can return FLAC__StreamMetadata_VorbisComment*
That would avoid the need for malloc and copy here.

I have reorganised the code according to other comments, and added the free to the character arrays that will be used.

ojw28 · 2019-07-09T16:25:51Z

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

@@ -110,6 +110,32 @@ DECODER_FUNC(jobject, flacDecodeMetadata, jlong jContext) {
                        streamInfo.total_samples);
 }

+DECODER_FUNC(jobject, flacDecodeVorbisComment, jlong jContext) {


For this to return something useful, does it rely on flacDecodeMetadata having been called previously (and specifically, on the call to context->parser->decodeMetadata() made on L94 above)?

If so, I think it would be much cleaner to rename FlacDecoderJni.decodeStreamInfo -> FlacDecoderJni.decodeMetadata and FlacStreamInfo -> FlacMetadata, and put the Arraylist<String> vorbisComment inside FlacMetadata

Here you'd pass the vorbisComment back as part of flacDecodeMetadata above, and the need for the separate flacDecodeVorbisComment would go away.

What do you think?

Yes, it does depend on the call to context->parser->decodeMetadata(). I will make this change and remove the separate call to flacDecodeVorbisComment.

Shall I push the refactor as a separate commit? Would it make the review easier?

Just adding separate commits as the code evolves is fine (and better than squashing + force pushing I think).

ojw28 · 2019-07-09T16:45:08Z

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

@@ -116,6 +126,8 @@ class FLACParser {
  const FLAC__StreamMetadata_SeekTable *mSeekTable;
  uint64_t firstFrameOffset;

+  bool mVorbisCommentValid;


nit: Can we keep mVorbisCommentValid and mVorbisComment grouped together, maybe just under the mStreamInfo... params, with a similar comment to those.

ojw28 · 2019-07-09T22:07:11Z

...y/core/src/main/java/com/google/android/exoplayer2/metadata/vorbis/VorbisCommentDecoder.java

+import java.util.ArrayList;
+
+/** Decodes vorbis comments */
+public class VorbisCommentDecoder {


nit: Please make classes final where possible, as it helps reduce our API surface. Ditto elsewhere.

Also, all other XXXDecoder classes in the metadata package implement MetadataDecoder, so this is slightly confusing. Given the data is already half decoded when it comes out of the JNI layer, it might be simpler just to merge this code into FlacDecoderJni or FlacExtractor.

I will remove this class. I will also see how I can still run the tests without this.

Would making decodeVorbisComments a public static method in the StreamMetadata (previously StreamInfo) be acceptable?

You could pass the ArrayList<String> to the StreamMetadata constructor, have the constructor do the decoding, and have StreamMetadata hold the decoded Metadata directory rather than the ArrayList<String>.

do the decoding as part of the StreamMetadata constructor, and then have StreamMetadata hold the decoded Metadata directly rather than the ArrayList<String>.

The test would then be building a StreamMetadata and checking that the metadata field after construction is as expected.

ojw28 · 2019-07-09T22:09:11Z

...y/core/src/main/java/com/google/android/exoplayer2/metadata/vorbis/VorbisCommentDecoder.java

+   * @return A {@link Metadata} structure with the vorbis comments as its entries.
+   */
+  public Metadata decodeVorbisComments(@Nullable ArrayList<String> metadataStringList) {
+    if (metadataStringList == null || metadataStringList.size() == 0) {


nit: Use .isEmpty() rather than size() == 0. Ditto below.

ojw28 · 2019-07-09T22:10:40Z

...y/core/src/main/java/com/google/android/exoplayer2/metadata/vorbis/VorbisCommentDecoder.java

+    for (String commentEntry : metadataStringList) {
+      String[] keyValue;
+
+      keyValue = commentEntry.split(SEPARATOR);


I suspect that it's allowed to have the = character in the value. In other words KEY=VAL=UE is probably valid. In which case you should be looking the first instance of the separator only. You may find Util.splitAtFirst helpful for this.

If you agree, it would be good to have a test case for this as well.

ojw28 · 2019-07-09T22:11:40Z

...ary/core/src/main/java/com/google/android/exoplayer2/metadata/vorbis/VorbisCommentFrame.java

+/** Base class for Vorbis Comment Frames. */
+public class VorbisCommentFrame implements Metadata.Entry {
+
+  /** The frame key and value */


When generating HTML Javadoc, this is going to attach "The frame key and value" to key, and nothing to value. Could you document each of them separately?

googlebot · 2019-07-11T04:20:18Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

vavadhani · 2019-07-11T06:34:24Z

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

@@ -116,6 +126,8 @@ class FLACParser {
  const FLAC__StreamMetadata_SeekTable *mSeekTable;
  uint64_t firstFrameOffset;

+  bool mVorbisCommentValid;


vavadhani · 2019-07-11T06:34:56Z

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

+        for (FLAC__uint32 i = 0; i < vc.num_comments; ++i) {
+          FLAC__StreamMetadata_VorbisComment_Entry vce = vc.comments[i];
+          if (vce.entry != NULL) {
+            std::string comment(reinterpret_cast<char *>(vce.entry),


Removed the mallocs here and the free. Using std::vector and std::string instead.

vavadhani · 2019-07-11T06:35:54Z

@ojw28 @harishdm @umangsaini I've made some changes, please take a look.

ojw28

Thanks for the changes! This looks good to merge. I put some comments inline for future reference, but there's no need to push any changes. We'll take care of them as part of the process we use to get the change merged.

ojw28 · 2019-07-11T17:26:59Z

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

@@ -172,6 +172,22 @@ void FLACParser::metadataCallback(const FLAC__StreamMetadata *metadata) {
    case FLAC__METADATA_TYPE_SEEKTABLE:
      mSeekTable = &metadata->data.seek_table;
      break;
+    case FLAC__METADATA_TYPE_VORBIS_COMMENT:
+      if (!mVorbisCommentValid) {
+        FLAC__StreamMetadata_VorbisComment vc = metadata->data.vorbis_comment;


nit (don't update): Avoid shortened variables names (e.g. use vorbisComment rather than vc).

ojw28 · 2019-07-11T17:27:52Z

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

+   * @param channels Number of channels of the FLAC stream.
+   * @param bitsPerSample Number of bits per sample of the FLAC stream.
+   * @param totalSamples Total samples of the FLAC stream.
+   * @param vorbisCommentList An {@link ArrayList<String>} that contains vorbis comments, which will


nit (don't update): No need to document the type of the argument in the comment, since it's already documented by the method signature.

ojw28 · 2019-07-11T17:28:38Z

...re/src/test/java/com/google/android/exoplayer2/metadata/vorbis/VorbisCommentDecoderTest.java

+
+/** Test for {@link FlacStreamMetadata}'s conversion of {@link ArrayList} to {@link Metadata}. */
+@RunWith(AndroidJUnit4.class)
+public final class VorbisCommentDecoderTest {


nit (don't update): Should be FlacStreamMetadataTest now (and in the util package).

PiperOrigin-RevId: 257668797

Add vorbis comments support to flac extractor

77e1e4c

Decode and add vorbis comments from the flac file to metadata. google#5527

googlebot added the cla: no label Jul 9, 2019

ojw28 added cla: yes cla: no and removed cla: no cla: yes labels Jul 9, 2019

harishdm reviewed Jul 9, 2019

View reviewed changes

ojw28 reviewed Jul 9, 2019

View reviewed changes

vavadhani force-pushed the bug-5527 branch from 3b480e0 to f6892de Compare July 11, 2019 04:20

googlebot added cla: yes and removed cla: no labels Jul 11, 2019

vavadhani added 2 commits July 11, 2019 11:43

Clean up vorbis comment extraction

fb1f91b

Refactor FlacStreamInfo to FlacStreamMetadata

4b776ff

vavadhani force-pushed the bug-5527 branch from f6892de to 4b776ff Compare July 11, 2019 06:19

vavadhani commented Jul 11, 2019

View reviewed changes

ojw28 reviewed Jul 11, 2019

View reviewed changes

ojw28 merged commit 4b776ff into google:dev-v2 Jul 14, 2019

ojw28 added a commit that referenced this pull request Jul 14, 2019

Merge pull request #6151 from ittiam-systems:bug-5527

56a922c

PiperOrigin-RevId: 257668797

ojw28 added a commit that referenced this pull request Jul 28, 2019

Merge pull request #6151 from ittiam-systems:bug-5527

bba0a27

PiperOrigin-RevId: 257668797

google locked and limited conversation to collaborators Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add vorbis comments support to flac extractor #6151

Add vorbis comments support to flac extractor #6151

vavadhani commented Jul 9, 2019 •

edited

Loading

googlebot commented Jul 9, 2019

vavadhani commented Jul 9, 2019

ojw28 commented Jul 9, 2019

harishdm Jul 9, 2019

vavadhani Jul 10, 2019

ojw28 Jul 9, 2019

vavadhani Jul 10, 2019

ojw28 Jul 10, 2019

ojw28 Jul 9, 2019

vavadhani Jul 11, 2019

ojw28 Jul 9, 2019

vavadhani Jul 10, 2019

vavadhani Jul 10, 2019

ojw28 Jul 10, 2019

ojw28 Jul 9, 2019

ojw28 Jul 9, 2019

ojw28 Jul 9, 2019

vavadhani Jul 11, 2019

googlebot commented Jul 11, 2019

vavadhani Jul 11, 2019

vavadhani Jul 11, 2019

vavadhani commented Jul 11, 2019 •

edited

Loading

ojw28 left a comment

ojw28 Jul 11, 2019

ojw28 Jul 11, 2019

ojw28 Jul 11, 2019

Add vorbis comments support to flac extractor #6151

Add vorbis comments support to flac extractor #6151

Conversation

vavadhani commented Jul 9, 2019 • edited Loading

googlebot commented Jul 9, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

vavadhani commented Jul 9, 2019

ojw28 commented Jul 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vavadhani commented Jul 11, 2019 • edited Loading

ojw28 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vavadhani commented Jul 9, 2019 •

edited

Loading

vavadhani commented Jul 11, 2019 •

edited

Loading