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

Fix playing back audio files & reading music metadata #1240

Merged
merged 24 commits into from Apr 19, 2018

Conversation

4 participants
@Borewit
Copy link
Member

commented Sep 17, 2017

Replaced musicmetadata with music-metadata module. The original one appears to be no longer maintained.

Fixes bug #1320, playing back audio files broken: caused by stream incompatibility between musicmetadata & readable-stream.

Should be faster, more stable is able to read more metadata and format information.
Supports additional audio formats and metadata tag headers.

Added audio format & some release information to the lay screen.

You could also use the library to retrieve album art from the audio files.

Borewit added some commits Sep 17, 2017

Add audio format information to play screen.
Extended album information with release lebel if available.
Make use of music-metadata module (musicmetadata appears to be no longer maintained)
Add catalognumber in addition of the release label.
Updated to music-metadata in order to recognize MIME-type: audio/x-flac

@Borewit Borewit closed this Sep 17, 2017

@Borewit Borewit reopened this Sep 17, 2017

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Feb 4, 2018

Would be great if one can review this proposal.

Borewit added some commits Feb 5, 2018

Fix bug #1320 by update music-metadata.
Bug was caused by an interoperability between then-read-stream and
readable-stream.
Use direct file access, if the individual file has completed download…
…ing.

Will allow slightly faster reading of music-metadata.

@Borewit Borewit changed the title Update music-metadata depenency Fix playing back audio files & reading music metadata Feb 6, 2018

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2018

Playing Opus audio and demonstrating visualization of its metadata:
metadata opus

Borewit added some commits Feb 19, 2018

Add comments to metadata media overlay.
Adjust the label element width to 120px to be able to fit in the text: 'Comments'.
Adjust the font-weight of the comments & format value to 'normal'.
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Feb 24, 2018

Added separate elements for year & album release information:

metadata with year release and comments

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2018

@codealchemist: can you please review my commits?

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2018

@codealchemist:

If we can't get metadata maybe we can show the filename instead.
That should already be the case:

const title = common.title ? common.title : fileSummary.name

Recreated a torrent using the same test file, but stripped of all metadata:
ogg-al-fatihah-no-metadata

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 3, 2018

@DiegoRBaquero:

can you include automated tests?

Just to be on the same page, it is not something new I am introducing here. Although I did add additional fields in the player page, I believe it fixes and improves existing functionality. As far as I have seen nothing was unit tested yet. Given this context, I think it is a bit odd to require unit test prior to merge, but I do fully understand it is desired

The music-metadata is heavily unit tested, also the ogg file which was causing a problems has been added as a unit test.

But what you are looking for is probably some extension of the test-integration demonstrating the metadata resolving is working, right?

I don't mind to look into it, problem is that I did not manage get the tests running.

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2018

@DiegoRBaquero: I am one step further with the test-integration. At least I can run the test-audio.js which actually did capture some metadata output testing via screenshot validation. I will update those accordingly.

Fixed the win32 version of the test-integration/test-audio: updated s…
…creenshots to match new meta-data.

The format value has been to partially transparent because the bit-rate is not stable.
This is due to the fact these MP3 are encoded with Variable Bit-Rate (VBR),and they are lacking a "Xing" header, providing VBR encoding details.
Related to issue #1320 & #1240.
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2018

@DiegoRBaquero : I noticed βTorrent can you use a boost as well in audio playback functionality 😉

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2018

@codealchemist can you please generate the darwin screenshots for me.

I tried to generate the OSX screenshots for the integration tests on VIrtualBox. I did not succeed. I set the resolution to 2304x1440, which I think is equivalent to the 2016 12" Macbook. I guess there is some (retina) scaling factor into play which causes my screen dumps from OSX to be to small.

I can add the transparency parts; if you just push the failed images, that would be great.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

commented Mar 5, 2018

Works for me now.

@Borewit What would you want to include in BTorrent? The streaming is pretty much restricted to the native browser functionality.

@Borewit

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2018

@DiegoRBaquero, in a couple of days I will open an issue on your repo with some ideas.

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

Retested with the problematic OGG file and now it works alright :)
image

I think there's still one problem with MP3 files that didn't finish downloading.
Looks like metadata is not yet available and we're displaying a blank screen.
image

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

Also, related to unfinished MP3 files: when the file finishes downloading it's not refreshing its metadata, which is available if I go back to the list and play it again.

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

An unfinished MP3 file, which shows a blank screen on WebTorrent Desktop, when opened with OSX Preview tool displays metadata:
image

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

Just tested reading that incomplete MP3 file with music-metadata on a unit test I'm adding and the content looks correct:

  Parsing MPEG / ID3v1
    should handle incomplete MP3 file
METADATA: { format:
   { lossless: false,
     dataformat: 'mp3',
     bitrate: 64000,
     sampleRate: 22050,
     numberOfChannels: 2,
     codecProfile: 'CBR',
     numberOfSamples: 95910336,
     duration: 4349.6751020408165,
     tagTypes: [ 'ID3v2.3', 'ID3v1.1' ] },
  native: undefined,
  common:
   { track: { no: null, of: null },
     disk: { no: null, of: null },
     picture: [ [Object] ],
     title: 'Al-An\'am',
     year: 2007,
     comment: [ 'www.TvQuran.com' ],
     album: 'www.TvQuran.com',
     genre: [ 'www.TvQuran.com' ],
     artist: 'Mishary Alafasi - www.TvQuran.com',
     artists: [ 'Mishary Alafasi - www.TvQuran.com' ] } }
@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2018

@Borewit, BTW, this is really cool:
image

Nice integration and great coverage ;)

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

commented Mar 8, 2018

This is looking really promising.

Pass file length parameter to music-metadata parser.
In some cases this is required to calculate the duration.
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2018

@feross: Can you please merge this one?

@codealchemist
Copy link
Contributor

left a comment

There are still some cases where a blank screen is shown for a period of time until metadata is successfully downloaded, but it's not that bad and I think I can easily add a fix to display the file name while metadata is being downloaded.

// otherwise stream
: mm.parseStream(file.createReadStream(), file.name, options)

onMetaData

This comment has been minimized.

Copy link
@codealchemist

codealchemist Apr 19, 2018

Contributor

For MP3 files it seems like this promise is not resolved until the whole file is downloaded.
But the ID3 tags are already available.
Forcing onMetaData to be mm.parseFile(path.join(torrent.path, file.path), options) properly loads metadata on tested files.

We might need to check how ID3 tags are being read from the file.
I think we could load them before the full file is downloaded, at least on same cases.

Also, we might want to add a loading indication if we need to wait for the whole file to be downloaded to update the metadata.
Probable we can show the file name and an indication that metadata is being loading.

Cheers!

@codealchemist codealchemist merged commit afd7f6e into webtorrent:master Apr 19, 2018

2 checks passed

Node Security No known vulnerabilities found
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Borewit

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2018

Fantastic news, finally merged yeah!!

Thank you so much @codealchemist.

@DiegoRBaquero

This comment has been minimized.

Copy link
Member

commented Apr 19, 2018

@feross we need a new release

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2018

I think we should wait until #1361 gets reviewed and merged before a new release.
:)

Thanks @DiegoRBaquero !

@feross

This comment has been minimized.

Copy link
Member

commented Apr 20, 2018

Will wait until #1361 is merged before doing a new release. Does someone want to take a stab at PR'ing the changelog for the next version?

@codealchemist

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2018

I'll take the stab at it.
:)

@lock lock bot locked as resolved and limited conversation to collaborators Jul 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.