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

Error: expected frame header but was not found #193

Closed
zeke opened this issue Jun 19, 2017 · 23 comments
Closed

Error: expected frame header but was not found #193

zeke opened this issue Jun 19, 2017 · 23 comments
Assignees
Labels
Milestone

Comments

@zeke
Copy link
Member

zeke commented Jun 19, 2017

I see this in the main console when I try to import my music directory:

~/git/forks/hyperamp master ⇣ 1d 59m 13s
❯ npm start

> [email protected] start /Users/z/git/forks/hyperamp
> electron main

scanning /Users/z/Google Drive/Music
Error: expected frame header but was not found
    at seekFirstAudioFrame (/Users/z/git/forks/hyperamp/node_modules/musicmetadata/lib/id3v2.js:174:21)
    at /Users/z/git/forks/hyperamp/node_modules/musicmetadata/lib/id3v2.js:80:18
    at emitData (/Users/z/git/forks/hyperamp/node_modules/strtok2/lib/index.js:419:20)
    at Stream.dataListener (/Users/z/git/forks/hyperamp/node_modules/strtok2/lib/index.js:482:11)
    at emitOne (events.js:96:13)
    at Stream.emit (events.js:188:7)
    at Stream.<anonymous> (/Users/z/git/forks/hyperamp/node_modules/musicmetadata/lib/index.js:69:13)
    at Object.onceWrapper (events.js:290:19)
    at emitOne (events.js:96:13)
    at Stream.emit (events.js:188:7)
2017-06-18 20:06:47.693 Electron Helper[88470:5873773] Couldn't set selectedTextBackgroundColor from default ()
@bcomnes
Copy link
Contributor

bcomnes commented Jun 19, 2017

Cool! I'll have to hunt down where this is throwing.

@bcomnes bcomnes self-assigned this Jun 19, 2017
@ungoldman
Copy link
Member

Seeing the same problem after pulling down latest master. Tried clearing out old configs too. Will try to figure out how to get it working again this evening.

@bcomnes
Copy link
Contributor

bcomnes commented Jun 23, 2017

I'm pretty sure these are just logged as warnings. If you are doing a large scan, wait it out a few mins as 15k tracks can take a few mins. We need to improve the feedback that something is actually happening, and provide a partially updated interface ideally. For now though, this shouldn't be a total show stopper.

@zeke
Copy link
Member Author

zeke commented Jun 23, 2017

Ok I will give it another try and leave it running.

@zeke
Copy link
Member Author

zeke commented Jun 23, 2017

Ok I just let it run for over an hour. I got the main process error right away again. No sign of anything happening in the UI, though nothing seems to be frozen. No errors in the renderer console.

@bcomnes
Copy link
Contributor

bcomnes commented Jun 23, 2017

It shouldn't take over an hour, only a few mins in my runs. I'll poke at it!

@zeke
Copy link
Member Author

zeke commented Jun 23, 2017

I let it run all night for good measure. No noticeable changes.

@bcomnes
Copy link
Contributor

bcomnes commented Jun 23, 2017

Sounds like its getting hung up somewhere. I'll improve the feedback of the scanning process and maybe it will help track down the source of the hangup.

@ungoldman
Copy link
Member

Definitely not just logged errors. The error halts the scanning process. Coming from a returned callback in musicmetadata

@bcomnes
Copy link
Contributor

bcomnes commented Jun 23, 2017

I have a feeling metadata parsing (end eventually editing) is going to be a gigantic only-partially solved problem in JS land.

@zeke
Copy link
Member Author

zeke commented Jun 23, 2017

metadata parsing (end eventually editing) is going to be a gigantic partially solved problem in JS land.

cc @0x00A :)

@bcomnes
Copy link
Contributor

bcomnes commented Jun 25, 2017

Quick fix: Just handle the errors slightly differently. The metadata scanner seems to fail in a number of unknown ways still that we need to sort out at some point.

Fix lives in the HEAD of master right now: 5ded061

If you have time, give it another shot. If you are running from the cli, you will see scanning progress logs during the scan. We probably want a log viewer in the main window that shows errors and progress detail updates.

@bcomnes
Copy link
Contributor

bcomnes commented Jun 25, 2017

Also, I just noticed we are using the less robust metadata parsing library in the main music scan (musicmetadata). It looks like we should switch to using music-metadata since that appeared to be be a bit more robust, but also totally changes the shape of the scanned data.

This will require a bit more research.

@bcomnes bcomnes added the bug label Jun 25, 2017
@bcomnes bcomnes modified the milestone: Winamp 2 Jun 25, 2017
@zeke
Copy link
Member Author

zeke commented Jun 26, 2017

Okay I just pulled latest master, npm i && npm start, and I see tons of filenames going by. Yay!

But then.. It hangs.

The last file is:

Scanning /Users/z/Google Drive/Music/iTunes/iTunes Music/Rock & Roll/Jefferson Airplane/1966 - Takes Off/Jefferson Airplane - 05 - Tobacco Road.mp3

A song which I'm embarrassed to say I don't know by name. Checked it out and it's only 4K and won't play in macOS Finder thingy. Here's the mp3 itself: https://cloudup.com/cfhoTNipZDU

Deleted the file and starting again. So far so good...

@zeke
Copy link
Member Author

zeke commented Jun 26, 2017

done scanning. found 22335 tracks

🎉

@bcomnes
Copy link
Contributor

bcomnes commented Jun 26, 2017

Hrmm... weird.. Perhaps a set-timeout is needed. EDIT I see its right there.

@bcomnes
Copy link
Contributor

bcomnes commented Jun 26, 2017

screen shot 2017-06-25 at 5 59 53 pm

@bcomnes
Copy link
Contributor

bcomnes commented Jun 26, 2017

Does cloudup have copyright / contentID?

@bcomnes
Copy link
Contributor

bcomnes commented Jun 26, 2017

@ungoldman will you try again too?

@ungoldman
Copy link
Member

ungoldman commented Jun 26, 2017

@bcomnes just tried, works for me now. 🎉

There's no visual indication that it's scanning outside of logs though, we might want to throw a loader in for now and maybe have some more interesting feedback on scanning in progress later.

edit: looks like you already noted that here: #192 (comment) 💯

@ungoldman
Copy link
Member

Also clearing out previous config seems to help from time to time. Related PR: #198

@bcomnes
Copy link
Contributor

bcomnes commented Jun 26, 2017

Thanks for the data points everyone :D

I landed #198 and I think we should also have a front-end button to reset the config, essentially as a low-rent migration script users can run from the front-end. Will open that soon.

As for scanning feedback I opened: #192 I was thinking that one step forward on that would be to merge the + and the gear icon, and when scanning, have the gear spin, and stop when its done.

@bcomnes
Copy link
Contributor

bcomnes commented Oct 23, 2017

Closing in favor of #185

@bcomnes bcomnes closed this as completed Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants