Asynchronous music-metadata updates while streaming #1449

Borewit · 2018-08-13T09:52:31Z

Implementation of asynchronous tag update events (send each time music-metadata founds a new generic tag or format change while it is parsing the downloaded stream build up from the torrent chunks).

Solution for #1340: Improve the music metadata reading and updating mechanism

Add feature which allow async parser subscription:
- Issue: Add feature which allow async parser subscription Borewit/music-metadata#125
- Pull request: Feature/async updates Borewit/music-metadata#126
- Branch: feature/async-updates
Review async concept
Release version music-metadata with async observer API included
Ready for final review

…ating. Previously, they were only installed when the preference was changed. This caused the handlers to point to non-existing files after updates occurred and older versions were removed by Squirrel. Closes #791, #911.

Borewit · 2018-08-13T10:03:38Z

@codealchemist Can you please review these changes?

Note that the dependency of music-metadata is not updated yet.
Since it a fairly large change to music-metadata, I would like have it reviewed prior to merging it.

At this time you need to checkout feature/async-updates branch of music-metadata, which is still a pending pull request.

You can yarn to link to your local music-metadata branch.

Borewit · 2018-08-13T10:25:43Z

Partial music-metadata update:

Few seconds later:

codealchemist · 2018-08-15T03:41:14Z

Oh, I can't play any files on first try.
But it doesn't throw any errors; strange.
Tomorrow I'll try again fresh, I'm too sleepy now.

Just wanted to say this is awesome stuff!
And I'm looking forward to testing it.
:)

codealchemist · 2018-08-15T03:42:58Z

BTW, I installed music-metadata from your branch like this:
npm i Borewit/music-metadata#feature/async-updates

Borewit · 2018-08-15T07:58:48Z

@codealchemist:

I am looking forward to your feedback!

npm i Borewit/music-metadata#feature/async-updates

I guess that is not working, because the TypeScript in music-metadata is not getting compiled.

I use yarn, to link the local checked out dependency.

git clone https://github.com/Borewit/music-metadata.git
cd music-metadata
git checkout feature/async-updates
yarn install
yarn build
yarn link
cd ../webtorrent-desktop
yarn link music-metadata

Borewit · 2018-08-15T08:21:24Z

This is the test set I am currently using: Audio torrent test set v01.zip

The MPEG-4 Audio Book File in 'Glories of Ireland 1801 librivox (audio book).torrent', are not working yet. That is why I started:

Reuse media extensions #1450: adding the .m4b extension, which I thought I had done before...
Failed to parse MPEG-4 Audio Book File (m4b) Borewit/music-metadata#127 which is marked as bug, but it is in fact a workaround for an ugly tag appearing in these m4b file.

As a reminder for an entry on the change-log and to ensure it got tested I created: #1452

Something else I noticed: suspiciously frequently, the track number is not visualized.
Update: issue confirmed, created fix: #1454

…e-music-metadata

… defined (common.track.of === null).

…track, disk, album, format

codealchemist

I tested with mp3 and ogg and it's working, yay!
I saw 13 async calls which initially looked like too many, but we might not be using all the data we get.
Didn't have time to check that yet.

Great work @Borewit !

codealchemist · 2018-08-16T11:33:48Z

src/renderer/webtorrent.js

+    skipCovers: true,
+    fileSize: file.length,
+    observer: event => {
+      console.log(`async-audio-metadata-update: file='${file.name}', type=${event.tag.type}, tag-id=${event.tag.id}`)


Maybe we can remove this extra logging before merging.
:)

codealchemist · 2018-08-16T11:36:47Z

src/renderer/pages/player-page.js

@@ -216,9 +216,7 @@ function renderAudioMetadata (state) {
  const elems = []

  // Audio metadata: artist(s)
-  const artist = common.albumartist || common.artist ||
-    (common.artists && common.artists.filter(function (a) { return a }).join(', ')) ||


common.artists array is not available anymore?

It is still available, but common.artist is already designed to summarize the artist(s) in a single string.
Either derived from common.artists or directly from the metadata.

In other words, the code removed is already done in music-metadata.

Borewit · 2018-08-16T12:10:11Z

I saw 13 async calls which initially looked like too many, but we might not be using all the data we get.

There maybe indeed calls now which do not effect any visual change because the metadata triggering the call is not used. I thought about this, and assumed these callback were not to expensive. But the source of all evil in the world are assumptions ;-). To keep things simple I decided not to filter the events, but that can be done if you think it is worth it.

#1452 Fix for playing some '.m4b' files

…re/async-update-music-metadata

codealchemist · 2018-08-17T11:32:33Z

About having extra async calls I think it's ok.
If we want to do further optimizations in the future maybe we can tell music-metadata the fields we're interested about to only do the callback for those.
This is really good already!

codealchemist

Looking good!
Thanks!

codealchemist · 2018-08-17T11:38:15Z

Sample OGG file properly getting async metadata:

Borewit · 2018-08-17T14:25:02Z

@codealchemist Thanks for reviewing, I think this one made a good difference.

I just found out that there are some cases where events are not triggered yet. Which may not be even noticeable for the end user, Fixed.

If we want to do further optimizations in the future maybe we can tell music-metadata the fields we're interested about to only do the callback for those.

I am not so sure about putting the filter in music-metadata. I don't think that is any better then just do something with the event which we are interested in. Callbacks are generated in music-metadata, with or without one listening in the app, because conversion and merging has to take place to maintain the common model. I would go for switch (sorry @feross, I know you hate them ;-) on the event.tag properties. Maybe it is more beneficial to wire more detailed screen updates with corresponding event updates. I have hardly any experience with React btw.

…gered.

…metadata' and 'fix/audio-track-nr' of https://github.com/Borewit/webtorrent-desktop into feature/async-update-music-metadata

codealchemist · 2018-08-19T00:56:15Z

src/renderer/lib/migrations.js

  // Config is now on the new version
  state.saved.version = config.APP_VERSION
 }

+// Whenever the app is updated,  re-install default handlers if the user has
+// enabled them.
+function installHandlers (saved) {


Maybe this enhancement is worth its own PR.
But if the other folks agree we can add it here.
Really nice catch BTW :)

…adata

# Conflicts: # src/renderer/webtorrent.js

Closes #1478

Borewit · 2018-09-26T05:39:06Z

@mathiasvr this one should be good for merging as well.

mathiasvr · 2018-09-26T15:10:44Z

Maybe it is not relevant to this PR, but doesn't the metadata streaming force webtorrent to download the file sequentially, even if the user wants to seek to a position later in the file?

Anyway, I think we can merge this and consider potential optimizations later.

A couple of things:

Could you remove the commit from Ensure that default file/protocol handlers are re-installed after updating. #997, I think it should be handled separately.
We could maybe update music-metadata to the latest version?

Borewit · 2018-09-26T18:31:53Z

Maybe it is not relevant to this PR, but doesn't the metadata streaming force webtorrent to download the file sequentially, even if the user wants to seek to a position later in the file?

Not sure if the metadata is also driving the sequential download. But if metadata doesn't trigger it, the audio player will do for sure will, despite the fact that you want to start playing half way for multiple reasons:

The header information is stored at the beginning of the file.
The audio player will read the metadata chunks as well
The Node stream does not provide a mechanism to jump in the stream

Although it is depending on the file format and provided settings, music-metadata doesn't read necessary to the end of the stream, in many cases, a relative small portion of the beginning of the stream is sufficient.

I have the impression the sequential download is already prioritized at the moment you navigate to the audio track "page".

For sure this PR doesn't change the way the audio metadata is read, that's the same is as before. It starts to display metadata during parsing.

But if you see a way how we can do things smarter, I will look into it.

Could you remove the commit from #997, I think it should be handled separately.

Regarding that commit, for sure I did not inject that on purpose.
I checked my local branch and I cannot find that commit. I deleted my local branch, checked it out again, it is not there. Neither I can find it in the commit history on the git repo.

Neither I see it ending up the changes, so I suppose is not part of the PR.

Maybe I made mistake at some point and falsified the history of the branch. So sorry.

We could maybe update music-metadata to the latest version?

Sounds good to me.

….0' into feature/async-update-music-metadata # Conflicts: # package.json

mathiasvr · 2018-09-26T19:19:35Z

Although it is depending on the file format and provided settings, music-metadata doesn't read necessary to the end of the stream, in many cases, a relative small portion of the beginning of the stream is sufficient.

Sounds good, in that case I do not see a problem. 👍

I checked my local branch and I cannot find that commit. I deleted my local branch, checked it out again, it is not there. Neither I can find it in the commit history on the git repo.

Yes that looks pretty weird? However I still see it in GitHub changes view, and also when manually pulling your branch to the master.

Can you try to rebase your branch? Otherwise if you check Allow edits from maintainers on this PR, I can try to do it.

Borewit · 2018-09-26T19:36:12Z

Oh I see what you mean, it is part of the change-set, overlooked that.

Allow edits from maintainers is and was granted Mathias, be my guest.

This is my last PR on a branch created in my clone, I will use webtorrent repo straight away in the future, much easier.

…fter updating." This reverts commit 39145b2.

Borewit · 2018-09-27T06:03:40Z

Thanks Mathias!

bnjmnt4n and others added 2 commits October 6, 2016 19:42

#1340: Switch to async metadata updates.

0e0b0e6

Borewit added enhancement meta labels Aug 13, 2018

Borewit self-assigned this Aug 13, 2018

Borewit requested a review from codealchemist August 13, 2018 09:52

Borewit changed the title ~~webtorrent/webtorrent-desktop#1340: Switch to async metadata updates.~~ Asynchronous music-metadata updates while streaming Aug 13, 2018

Borewit mentioned this pull request Aug 13, 2018

Improve the music metadata reading and updating mechanism #1340

Closed

1 task

Use fat arrow

198c62e

codealchemist mentioned this pull request Aug 15, 2018

Borewit dependency/update music metadata #1440

Merged

Borewit assigned codealchemist Aug 15, 2018

Borewit added 4 commits August 15, 2018 20:40

Merge remote-tracking branch 'origin/master' into feature/async-updat…

b185a62

…e-music-metadata

Fix issue track number not displayed if total number of tracks is not…

a30d804

… defined (common.track.of === null).

Add disk number in addition to track number.

c2594f5

Update order of audio properties from: album, track, disk, format to …

c7f30df

…track, disk, album, format

codealchemist reviewed Aug 16, 2018

View reviewed changes

Borewit added 2 commits August 16, 2018 21:00

#1340 Update music-metadata to 2.5.0, enabling async 'per' tag updates

b73db82

#1452 Fix for playing some '.m4b' files

#1340 Commented out the metadata event debug output.

8848534

Borewit mentioned this pull request Aug 16, 2018

Ability to play MPEG-4 Audio Book (.m4b) #1452

Closed

4 tasks

#1340 Remove line comment to get rid of max line length lint error

f925b1f

Borewit requested review from feross, DiegoRBaquero and dcposch August 16, 2018 19:14

Borewit added 2 commits August 16, 2018 22:40

Merge branch 'master' into feature/async-update-music-metadata

30276d5

Merge remote-tracking branch 'upstream/fix/audio-track-nr' into featu…

c6944a3

…re/async-update-music-metadata

codealchemist approved these changes Aug 17, 2018

View reviewed changes

Borewit added 3 commits August 17, 2018 16:55

Update music-metadata 2.6.0 to fix some async events are getting trig…

090b906

…gered.

Return JSX block.

e2448ca

Merge branches 'demoneaux/fix-handlers', 'feature/async-update-music-…

c85f331

…metadata' and 'fix/audio-track-nr' of https://github.com/Borewit/webtorrent-desktop into feature/async-update-music-metadata

codealchemist reviewed Aug 19, 2018

View reviewed changes

Borewit and others added 7 commits August 19, 2018 09:52

Get rid of third parameter which is replaced by CSS capitalize

4306569

Merge branch 'fix/audio-track-nr' into feature/async-update-music-met…

e508b13

…adata

Fixed error when value is undefined.

8e91840

Merge branch 'fix/audio-track-nr' into feature/async-update-music-met…

50bf7e1

…adata

Merge branch 'master' into feature/async-update-music-metadata

ee9bf1d

# Conflicts: # src/renderer/webtorrent.js

Update music-metadata dependency to 2.6.1.

e34edc2

fix(package): update music-metadata to version 3.1.0

db50e97

Closes #1478

Borewit requested a review from mathiasvr September 26, 2018 05:36

Merge remote-tracking branch 'upstream/greenkeeper/music-metadata-3.1…

f7719a7

….0' into feature/async-update-music-metadata # Conflicts: # package.json

Revert "Ensure that default file/protocol handlers are re-installed a…

78b71d3

…fter updating." This reverts commit 39145b2.

mathiasvr approved these changes Sep 26, 2018

View reviewed changes

mathiasvr merged commit 06566d3 into webtorrent:master Sep 26, 2018

lock bot locked as resolved and limited conversation to collaborators Dec 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asynchronous music-metadata updates while streaming #1449

Asynchronous music-metadata updates while streaming #1449

Borewit commented Aug 13, 2018 •

edited

Loading

Borewit commented Aug 13, 2018

Borewit commented Aug 13, 2018

codealchemist commented Aug 15, 2018

codealchemist commented Aug 15, 2018

Borewit commented Aug 15, 2018

Borewit commented Aug 15, 2018 •

edited

Loading

codealchemist left a comment

codealchemist Aug 16, 2018

codealchemist Aug 16, 2018

Borewit Aug 16, 2018

Borewit commented Aug 16, 2018

codealchemist commented Aug 17, 2018

codealchemist left a comment

codealchemist commented Aug 17, 2018

Borewit commented Aug 17, 2018 •

edited

Loading

codealchemist Aug 19, 2018

Borewit commented Sep 26, 2018

mathiasvr commented Sep 26, 2018

Borewit commented Sep 26, 2018 •

edited

Loading

mathiasvr commented Sep 26, 2018 •

edited

Loading

Borewit commented Sep 26, 2018 •

edited

Loading

Borewit commented Sep 27, 2018

Asynchronous music-metadata updates while streaming #1449

Asynchronous music-metadata updates while streaming #1449

Conversation

Borewit commented Aug 13, 2018 • edited Loading

Borewit commented Aug 13, 2018

Borewit commented Aug 13, 2018

codealchemist commented Aug 15, 2018

codealchemist commented Aug 15, 2018

Borewit commented Aug 15, 2018

Borewit commented Aug 15, 2018 • edited Loading

codealchemist left a comment

Choose a reason for hiding this comment

codealchemist Aug 16, 2018

Choose a reason for hiding this comment

codealchemist Aug 16, 2018

Choose a reason for hiding this comment

Borewit Aug 16, 2018

Choose a reason for hiding this comment

Borewit commented Aug 16, 2018

codealchemist commented Aug 17, 2018

codealchemist left a comment

Choose a reason for hiding this comment

codealchemist commented Aug 17, 2018

Borewit commented Aug 17, 2018 • edited Loading

codealchemist Aug 19, 2018

Choose a reason for hiding this comment

Borewit commented Sep 26, 2018

mathiasvr commented Sep 26, 2018

Borewit commented Sep 26, 2018 • edited Loading

mathiasvr commented Sep 26, 2018 • edited Loading

Borewit commented Sep 26, 2018 • edited Loading

Borewit commented Sep 27, 2018

Borewit commented Aug 13, 2018 •

edited

Loading

Borewit commented Aug 15, 2018 •

edited

Loading

Borewit commented Aug 17, 2018 •

edited

Loading

Borewit commented Sep 26, 2018 •

edited

Loading

mathiasvr commented Sep 26, 2018 •

edited

Loading

Borewit commented Sep 26, 2018 •

edited

Loading