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

Borewit dependency/update music metadata #1440

Merged
merged 6 commits into from
Jul 25, 2018

Conversation

codealchemist
Copy link
Contributor

Here we can review the update I propose to ensure we always get proper audio metadata.
image

@Borewit
Copy link
Member

Borewit commented Jul 23, 2018

Thanks for all the great work Alberto!
Ofcourse I would like to look into this, but I am traveling at the moment, not sure if I am able to soon.

I know you worked on retrieving the metadata on partial downloaded file.
?If insufficient data is available that may fail. If new data comes in, will that trigger additional attempts?

Recently I added the skipPostHeaders flag to options music-metadata. This avoids search for ID3v1 headers at which appear at the far end of file. Probaly a good idea to acitivate this one for files which are still downloading.

Copy link
Member

@DiegoRBaquero DiegoRBaquero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for fixing the standard issue

@Borewit
Copy link
Member

Borewit commented Jul 24, 2018

Thanks for reviewing Diego.

Copy link
Member

@Borewit Borewit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained, not able to rest run time at the moment. Changes are looking good to me.

@Borewit
Copy link
Member

Borewit commented Jul 25, 2018

Are you able to reproduce the issues I was experiencing before that change?

I am.
2018-07-25 unkown artist

@codealchemist
Copy link
Contributor Author

@Borewit Thank you dude!
Yeah, when new audio metadata is available a promise will be resolved and wt-audio-metadata event will be fired with the metadata. That will end up calling the torrentAudioMetadata method on torrent-controller, will update fileSummary.audioInfo and dispatch an update.

Regarding skipPostHeaders, maybe we can try downloading the final chunks of the file if we didn't find metadata on the first ones. Will that work? On the gif below the final chunks were already downloaded but metadata was available only after the whole file finished downloading.

webtorrent-desktop-incoming-audio-metadata

@codealchemist codealchemist merged commit cbbc61d into master Jul 25, 2018
@Borewit
Copy link
Member

Borewit commented Jul 25, 2018

After the merge, I can still reproduce the issue.

In case of a partial downloaded file, we can not safely assume we have read the metadata. Depending on how much is downloaded, and depending on the audio format, there may be none, some or all of the metadata available.
I think that is the root cause of the issue that the music-metadata is resolved with just some metadata.

Let's not worry about the post-headers (a term I invented for headers appearing at the end of the audio track, typically ID3v1 header) at this moment. Files with just an ID3v1 header are rare, and if that is the case it will still search for the header of nothing else is found.

I guess we need to be a mechanism which updates the metadata mutiple times while downloading, until we got all metadata available.

  1. So we need to figure out when we downloaded all metadata.
  2. Have mechanism how to trigger another attempt to read metadata, maybe after each new chunk but but within 0.5 seconds after the previous attempt.

Which is more or less coming back to issue #1340.

@feross feross deleted the Borewit-dependency/update-music-metadata branch July 25, 2018 17:12
@codealchemist
Copy link
Contributor Author

@Borewit if we can pass a callback to music-metadata to be called every time there's more metadata that might work.
As a probably simpler approach, we can read music metadata again once the file finishes downloading.

Which torrent are you using?
I'd like to run the same tests.
But probably in a few days, I'm staying at a hotel and the Internet connection here sucks.

Thanks!

@Borewit
Copy link
Member

Borewit commented Jul 26, 2018

That is more efficient approach indeed. I have to admit that this is a function I removed from the original module I cloned from. It will result in some challenges with mapping to the common structure, with duplicates and such. It also requires the stream has to be blocked where chunks are not available yet, but that is the way it works with video as well I suppose.

@Borewit
Copy link
Member

Borewit commented Jul 26, 2018

I don't have any torrents with me, neither I have a very extensive torrent test anywhere else. But it maybe worth to build a good test set.

@codealchemist
Copy link
Contributor Author

I retested and my change on playback-controller.js wasn't useful after all.
It works in the same way.
For mp3 and flac files we load metadata when it's available, this seems to work OK.
For ogg files we need to reload the file for metadata to be loaded (wait until it fully downloads, stop, play again).
I wasn't able to test with other file types.

Saw you've been working on other related PRs so I'll take a look there.
Thx!

@codealchemist
Copy link
Contributor Author

Enhancements continue on #1449.

@Borewit
Copy link
Member

Borewit commented Aug 15, 2018

Enhancements continue on #1449.

That nails the update mechanism.

@lock lock bot locked as resolved and limited conversation to collaborators Nov 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants