-
Notifications
You must be signed in to change notification settings - Fork 640
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
Update lazy loading branch with event/observer based metadata scanning #650
Update lazy loading branch with event/observer based metadata scanning #650
Conversation
Borewit
commented
Sep 2, 2018
- Replaced jsmediatags with music-metadata-browser.
- Media tags are now updated as soon as they are found.
…-metadata / strtok3.
…/*`. The relative URL used for 'Llama DJ Mike Llama' results in a MIME-type: `audio/mpeg; charset=UTF-8`.
# Conflicts: # config/webpack.common.js
…music-metadata
Pass MIME-type parameters to music-metadata.
# Conflicts: # js/actionCreators/files.js
719ce9a
to
49d4637
Compare
Still TODO Find a nice way to expose WebampLazy as: ```JavaScript import WebampLazy from 'webamp/lazy'; ```
This was originally removed in captbaritone#648 And accidentally re-added in captbaritone#639 due to a bad rebase.
}, | ||
plugins: [ | ||
new webpack.IgnorePlugin(/fs/, /file-type/, /debug/), // Ignore fs in music-metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping that with the introduction of music-metadata-browser
this stuff, and the relevant polyfills, could be handled inside music-metadata-browser
's build process.
# Conflicts: # js/actionCreators/files.js # js/fileUtils.js # js/index.js # js/webamp.js # js/webampLazy.js # package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good! A few comments. Also, I'm curious how we handle the kbps/channels/khz of preloaded tracks. Currently we lie about all these things, so a user does not need to fetch them, and also does not need to provide them themselves. I guess we need to add a few things:
- The ability to provide these pieces of data up-front. In the
initialTracks
option. - Ensure we handle the case where a track is playing but does not have audio metadata yet. (Maybe this already works, I haven't checked.
js/config.js
Outdated
metaData: { | ||
title: "The Cycle (Featuring Mista Mista)", | ||
artist: "CM aka Creative" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you're ready for a final review, please remove this commit.
type: source.name, | ||
size: source.size | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping this could move inside of music-metadata-browser
js/fileUtils.js
Outdated
}; | ||
} | ||
|
||
export async function genMediaTags(file, mm, observer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mm
is not a great variable name in my opinion. Maybe we could just pass in a parseStream
function?
js/webamp.js
Outdated
return supportsAudioApi && supportsCanvas && supportsPromises; | ||
} | ||
import JSZip from "jszip"; | ||
import mm from "music-metadata-browser"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather call it musicMetadata
than mm
.
package.json
Outdated
@@ -16,7 +19,7 @@ | |||
"analyze": "webpack --config=config/webpack.bundle-analyzer.js", | |||
"prepublishOnly": "npm run build-library", | |||
"serve": "http-server ./built", | |||
"start": "webpack-dev-server --config=config/webpack.dev.js", | |||
"start": "webpack-dev-server --env.DEBUG=*- --config=config/webpack.dev.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
package.json
Outdated
"babel-polyfill": "^6.22.0", | ||
"babel-preset-env": "^1.6.1", | ||
"babel-preset-react": "^6.24.1", | ||
"babel-runtime": "^6.26.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bad rebase. I don't think these are supposed to be here.
@@ -75,6 +87,7 @@ | |||
"eslint-plugin-prettier": "^2.2.0", | |||
"eslint-plugin-react": "^7.7.0", | |||
"file-loader": "^1.1.5", | |||
"filereader-stream": "^2.0.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hopeful that this and stream-http
can become dependencies of music-metadata-browser
and that Webamp won't need to know about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will have a look to directly support parsing from a browser File object.
I am not sure if that is a good idea to embed an http client. HTTP is world on it's own, there are many different http clients. If security for example is an important aspect, things can become tricky.
At the other hand, stream is a great abstraction, but it does not offer the ability to jump (read seek or fast forward in the data).
# Conflicts: # package.json
…as completed loading.
In this commit I changed the code so that the metadata is (re-)read 0516124 just before playing. This results a bit in a hick-up after hitting the play button. |
Shall I close this in favor of #634? |
Can be closed. |