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

Support "common" (and not only "native") for .webm files #835

Closed
martpie opened this issue Jun 24, 2021 · 6 comments · Fixed by #837
Closed

Support "common" (and not only "native") for .webm files #835

martpie opened this issue Jun 24, 2021 · 6 comments · Fixed by #837
Assignees
Labels
bug Bug, will addressed with high priority

Comments

@martpie
Copy link

martpie commented Jun 24, 2021

Feature Request

Hello 👋

Thank you again for this great library!

Debugging martpie/museeks#590, I realized that maybe a better fix would be to fix it at the package level, rather than the application level, do you think it would be do-able?

Sample files: https://drive.google.com/drive/folders/1m_VdewpLCW5SyKF3QT4n1FtS-D6Dd4iC

@Borewit
Copy link
Owner

Borewit commented Jun 24, 2021

If something needs to get fixed or augmented, this is the right place and welcome back @martpie.

So I basically see 2 tags which possibly could be mapped to common:

Webm Tag common
50:ARTIST albumartist
50:GENRE genre

I could not find any information what this 50: prefix means or where it comes from.

Foobar2000 seems to understand these tags so I followed their example. So I follow their example. That is why I propose to map 50:ARTIST to albumartis and not to artist

Borewit added a commit that referenced this issue Jun 24, 2021
@Borewit
Copy link
Owner

Borewit commented Jun 24, 2021

2277c90 would make the following test pass:

describe('#835', () => {

  it('should parse "Dance Money.webm"', async () => {

    const filePath = path.join(issue_path, '#835', 'Dance Money.webm');

    const {format, common, native} = await mm.parseFile(filePath, {duration: true});
    assert.strictEqual(format.container, 'EBML/webm', 'format.container');
    assert.strictEqual(format.codec, 'OPUS', 'format.codec');

    assert.deepStrictEqual(common.genre, ['hip hop'], 'common.genre');
  });

  it('should parse "Let Me Down Slowly.webm"', async () => {

    const filePath = path.join(issue_path, '#835', 'Let Me Down Slowly.webm');

    const {format, common, native} = await mm.parseFile(filePath, {duration: true});
    assert.strictEqual(format.container, 'EBML/webm', 'format.container');
    assert.strictEqual(format.codec, 'OPUS', 'format.codec');

    assert.strictEqual(common.title, 'Let Me Down Slowly', 'common.title');
    assert.strictEqual(common.albumartist, 'Alec Benjamin', 'common.albumartist');
  });

  it('should parse "tagged.webm"', async () => {

    const filePath = path.join(issue_path, '#835', 'tagged.webm');

    const {format, common, native} = await mm.parseFile(filePath, {duration: true});
    assert.strictEqual(format.container, 'EBML/webm', 'format.container');
    assert.strictEqual(format.codec, 'OPUS', 'format.codec');

    assert.isUndefined(common.title, 'common.title');
    assert.strictEqual(common.albumartist, 'son go ku', 'common.title');
  });

});

@Borewit Borewit self-assigned this Jun 24, 2021
@Borewit Borewit pinned this issue Jun 24, 2021
@Borewit
Copy link
Owner

Borewit commented Jun 25, 2021

Is this sufficient, did I miss anything @martpie?

@martpie
Copy link
Author

martpie commented Jun 25, 2021

I think it looks very good like that! @haidang666 what do you think?

@haidang666
Copy link

haidang666 commented Jun 26, 2021

look good, btw ffmpeg was used to edit that metadata, the 50: maybe related in the structure in webm

@Borewit Borewit added bug Bug, will addressed with high priority and removed enhancement labels Jun 26, 2021
@Borewit
Copy link
Owner

Borewit commented Jun 26, 2021

The 50: is caused by a bug in this Matroska parser, there is no default value type in these tags (type, is album, track etc...). It should fall back to album as the default, but it turns the type ID instead of the string value.

Another related bug I fixed in #836.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, will addressed with high priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants