Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sniff FLAC files correctly if they have ID3 headers #4055

Closed
tangmingscau opened this issue Mar 30, 2018 · 5 comments
Closed

Sniff FLAC files correctly if they have ID3 headers #4055

tangmingscau opened this issue Mar 30, 2018 · 5 comments
Assignees

Comments

@tangmingscau
Copy link

On my side, I have a FLAC music format music, which has a ID3 head on the head of the format signature, which leads to FlacExtractor.sniff judgment error. He thinks he is not FLAC music.

I will send this FLAC music to you by mailbox

@ojw28
Copy link
Contributor

ojw28 commented Apr 2, 2018

The content provided doesn't download for me. When I click the link I get a spinner indefinitely. Please can you attach a sample directly to your email, or else use a file sharing service that works reliably (e.g. DropBox / Google Drive).

Note that ID3 tags at the start of FLAC files is not really a thing. As per the FLAC FAQs:

... you should not expect any tags beside FLAC tags to be supported in applications; some implementations may not even be able to decode a FLAC file with ID3 tags

We'll take a look anyway if you're able to provide content we can access, and perhaps look into handling the case if it seems worthwhile.

@ojw28 ojw28 self-assigned this Apr 2, 2018
@tangmingscau
Copy link
Author

@ojw28 ojw28 changed the title FLAC music format judgment method is incompatible with FLAC music head with ID3 Sniff FLAC files correctly if they have ID3 headers Apr 3, 2018
@ojw28 ojw28 assigned botaydotcom and unassigned ojw28 Apr 3, 2018
@ojw28
Copy link
Contributor

ojw28 commented Apr 3, 2018

@botaydotcom - This fits nicely under the "extractor stuff" umbrella. Mp3Extractor has code for sniffing/parsing id3 metadata, so it's probably a case of copying/sharing that with FlacExtractor. We should probably actually parse the metadata and put it into the Format, add a FLAG_DISABLE_ID3_METADATA flag as in Mp3Extractor, and plumb that through DefaultExtractorsFactory.

Implementation points:

  • It's important to adhere to how Extractor.sniff is defined.
  • It should not be assumed that sniff will be called (it's valid to only call read, if the calling code knows that it already has the correct extractor).

@tangmingscau
Copy link
Author

Is it possible for me to understand that there will be a new version to solve this problem?
Thanks

@botaydotcom
Copy link
Contributor

Yes, there will be a new version to solve this problem. However, this will not be worked on very soon because I'm working some other tasks on hand (expect ~1 month).

ojw28 pushed a commit that referenced this issue Apr 10, 2018
Support parsing ID3 tags at the beginning of FLAC files, even though FLAC spec
does not require this.

GitHub: #4055.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=192127929
@ojw28 ojw28 closed this as completed Apr 10, 2018
ojw28 pushed a commit that referenced this issue May 7, 2018
LibFlac internally can skip ID3 tags correctly. Therefore, we don't need to keep track of the whole ID3 header section and skip through this section in Java code. We can just set the whole stream to the native library, and it will handle skipping ID3 tags correctly.

The only thing that the Java part need to do is peeking and parsing ID3 tags (if present), in order to populate the track format metadata.

GitHub: #4055.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=193327602
@google google locked and limited conversation to collaborators Aug 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants